Opened 17 years ago
Last modified 17 years ago
#678 new defect
File descriptor leak in stream.c
Reported by: | Owned by: | reimar | |
---|---|---|---|
Priority: | normal | Component: | streaming |
Version: | HEAD | Severity: | normal |
Keywords: | Cc: | ||
Blocked By: | Blocking: | ||
Reproduced by developer: | no | Analyzed by developer: | no |
Description
I noticed in file "stream.c" that a file descriptor isn't closed
before freeing the stream.
The patch is really trivial, please apply.
Index: stream/stream.c
===================================================================
--- stream/stream.c (revision 21536)
+++ stream/stream.c (working copy)
@@ -178,6 +178,7 @@
s->flags |= mode;
*ret = sinfo->open(s,mode,arg,file_format);
if((*ret) != STREAM_OK) {
+ if (s->fd >= 0) close(s->fd);
free(s->url);
free(s);
return NULL;
Attachments (1)
Change History (8)
comment:1 by , 17 years ago
comment:2 by , 17 years ago
I can reproduce the problem while streaming ASF over HTTP using vlc as
server. MPlayer first try the stream/http plugin then stream/asf when
it notices the content-type is "video/x-ms-asf". But for some reason
(maybe vlc bug?) the stream/asf pluging fails to parse the data.
MPlayer falls back to the stream/http plugin again and actually plays
the video. However the problem is I have three connections established
with the stream server instead of one.
This is a test URL :
http://84.96.219.141:8087
comment:3 by , 17 years ago
I've re-read the code of stream/http and stream/asf plugins and found
a number of return errors when the socket isn't closed.
comment:4 by , 17 years ago
Owner: | changed from | to
---|
Most fixed in one way or another. The changes to fixup_open and open_s1 seem to
be wrong though, the fd is not opened there so it should not be closed there
IMO.
comment:5 by , 17 years ago
Many thanks. I've run a "svn update" and most of the issues are fixed
with less lines of code. That's a very good work, and I've two minor
suggestions to make it even better.
The variable "stream->fd" may contain the value of a closed fd,
therefore I think it should be set to -1 on error. (as you did
in asf_streaming.c)
Index: stream/http.c
===================================================================
--- stream/http.c (revision 21572)
+++ stream/http.c (working copy)
@@ -857,6 +857,7 @@
err_out:
if (fd > 0) closesocket( fd );
+ stream->fd = -1;
res = -1;
http_free( http_hdr );
out:
In this case the variable "res" is written two times and it may be
confusing. The line below is not very useful because the macro
STREAM_UNSUPORTED is -1.
Index: stream/http.c
===================================================================
--- stream/http.c (revision 21572)
+++ stream/http.c (working copy)
@@ -844,7 +844,6 @@
break;
case 401: Authentication required
if( http_authenticate(http_hdr, url,
&auth_retry)<0 ) {
- res = STREAM_UNSUPORTED;
goto err_out;
}
redirect = 1;
comment:6 by , 17 years ago
I've just re-run a test with the current SVN (revision 21572). I've
still two open sockets when MPlayer plays an ASF over HTTP video.
It seems to me the problem is the function open_s1(). The connection
to the server is successfully established, but the test below is
true because the content-type is "video/x-ms-asf".
Index: stream/http.c
===================================================================
--- stream/http.c (revision 21572)
+++ stream/http.c (working copy)
@@ -905,6 +905,10 @@
if((seekable < 0) (*file_format == DEMUXER_TYPE_ASF)) { streaming_ctrl_free(stream->streaming_ctrl);
stream->streaming_ctrl = NULL;
+ if (stream->fd >= 0) {
+ closesocket(stream->fd);
+ stream->fd = -1;
+ }
return STREAM_UNSUPORTED;
}
My tests indicate that there's only one single open socket when
I apply this patch.
comment:7 by , 17 years ago
Should be all fixed, thanks for your efforts.
There unfortunately are more leaks and bugs to be found around there, e.g. I
noticed streaming_ctrl is not freed when playing multiple files.
All those network streams need a close functions, also because the current way
to decide whether close or closesocket needs to be used is broken (will use
closesocket for everything containing :, e.g. tv://, file:// etc.).
This is wrong, the stream "plugin" is responsible for cleaning up stuff if open
fails. With what kind of URL did this happen, at least stream_file.c seems
correct?