Opened 11 years ago

Last modified 11 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: Analyzed by developer:

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@… 11 years ago.
Fix fd leak in asf/http streaming

Download all attachments as: .zip

Change History (8)

comment:1 Changed 11 years ago by reimar

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 Changed 11 years ago by nbk@…

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

Changed 11 years ago by nbk@…

Fix fd leak in asf/http streaming

comment:3 Changed 11 years ago by nbk@…

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 Changed 11 years ago by reimar

  • 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 Changed 11 years ago by nbk@…

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 Changed 11 years ago by nbk@…

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 Changed 11 years ago by reimar

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.