Opened 17 years ago

Last modified 17 years ago

#678 new defect

File descriptor leak in stream.c

Reported by: nbk@… 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)

fd_leak_asf_http.patch (6.7 KB ) - added by nbk@… 17 years ago.
Fix fd leak in asf/http streaming

Download all attachments as: .zip

Change History (8)

comment:1 by reimar, 17 years ago

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?

comment:2 by nbk@…, 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

by nbk@…, 17 years ago

Attachment: fd_leak_asf_http.patch added

Fix fd leak in asf/http streaming

comment:3 by nbk@…, 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 reimar, 17 years ago

Owner: changed from r_togni@… to Reimar.Doeffinger@…

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 nbk@…, 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 nbk@…, 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 reimar, 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.).

Note: See TracTickets for help on using tickets.