[Home]

Summary:ASTERISK-14190: [patch] file convert leaks input file descriptor
Reporter:Jaco Kroon (jkroon)Labels:
Date Opened:2009-05-22 09:18:44Date Closed:2009-05-26 13:36:30
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Resources/res_convert
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) res_convert_fs_leak.patch
Description:when issueing a file convert it seems that asterisk is leaking the input file descriptor.  As a test I copied a .g729 file into /tmp (/tmp/test.g729), the rest follows:

gabriel ~ # ls -l /tmp/test.*
-rw-r--r-- 1 root root 3380 May 22 16:10 /tmp/test.g729
gabriel ~ # ls -l /proc/$(pidof asterisk)/fd/ | grep test
gabriel ~ # asterisk -rx "file convert /tmp/test.g729 /tmp/test.wav"
Converted /tmp/test.g729 to /tmp/test.wav in 12ms
gabriel ~ # ls -l /proc/$(pidof asterisk)/fd/ | grep test
lrwx------ 1 asterisk asterisk 64 May 22 16:10 179 -> /tmp/test.g729
gabriel ~ # ls -l /tmp/test.*
-rw-r--r-- 1 root     root      3380 May 22 16:10 /tmp/test.g729
-rw-r--r-- 1 asterisk asterisk 54124 May 22 16:12 /tmp/test.wav
gabriel ~ #

I'll take a peek at the code a little later, for now I just need to have it reported.  I don't think this was the case in 1.6.0.3 but I'll need to go find an installation to confirm.
Comments:By: Russell Bryant (russell) 2009-05-22 09:23:08

This is actually expected and intended behavior.

By default, if you do not specify an absolute path, Asterisk will look for files in the normal sounds directory.  So, the use case was if you got a new prompt, dropped it in there, and then ran the command a few times to get it converted into the other formats that you wanted.
By: Jaco Kroon (jkroon) 2009-05-25 04:57:12

This changed from 1.6.0.3 at which time I could issue a file convert without leaving the dangling file descriptor.

Based on the code in res/res_convert.c a file stream is opened (ast_readfile), a function called ast_readframe is then called on this stream, after a few iterations ast_closestream is called.

Looking at the code for ast_readfile I can't see anything strange, nor on ast_closestream.  Well, not by itself anyway, looking at ast_readframe there is a reference counter being incremented, this reference count is also checked in ast_closestream, since it's non-zero the stream is left alone and as a result the file descriptor is never closed, even though asterisk no longer keeps a reference to it anywhere.

I can't see how the current behavior of leaking file descriptors is intentional.  It causes asterisk to go belly up if too many file converts is issued (due to lack of file descriptor availability when attempting to open other files).  This has nothing to do with absolute vs relative path handling (if it does I fail to see how).
By: Jaco Kroon (jkroon) 2009-05-25 05:02:39

The exact eventual problem is shown as this:

[May 25 11:58:13] WARNING[10949]: pbx_spool.c:473 scan_thread: Unable to open directory /var/spool/asterisk/outgoing: Too many open files
[May 25 11:58:13] ERROR[10957]: acl.c:472 ast_ouraddrfor: Cannot create socket

It prevents clients from dialing.
By: Jaco Kroon (jkroon) 2009-05-25 05:39:32

Ok, I think I managed to figure out what's going on, please do confirm.  Based on a diff of 1.6.0.3 and 1.6.0.9 res/res_convert.c is identical.  However, main/file.c has changed a little.  In particular, there were changes made to ast_closestream, in particular, a large block of code was replaced with this:

+ if (ast_test_flag(&f->fr, AST_FRFLAG_FROM_FILESTREAM)) {
+ /* If this flag is still set, it essentially means that the reference
+ * count of f is non-zero. We can't destroy this filestream until
+ * whatever is using the filestream's frame has finished.
+ *
+ * Since this was called, however, we need to remove the reference from
+ * when this filestream was first allocated. That way, when the embedded
+ * frame is freed, the refcount will reach 0 and we can finish destroying
+ * this filestream properly.
+ */
+ ao2_ref(f, -1);
+ return 0;


Which is basically checking the same reference flag that is being incremented in ast_readframe.  The appropriate function apparently to decrement this reference counter is ast_filestream_frame_freed.

As a side note btw, the entire ast_closestream (unless test_flag has side effects) can be reduced to ao2_ref(f, -1); return 0;, however, that is not related to the issue at hand, I think.

I suspect the ast_filestream_frame_freed function needs to be called from somewhere, I'm just not 100 % sure from where.
By: Jaco Kroon (jkroon) 2009-05-25 06:03:20

Hi,

I just attached a patch that "cures" the bad behavior for me, I have (however) no idea about the correctness of the patch.  Please audit and provide feedback.  This is a rather critical issue for me at least.
By: Digium Subversion (svnbot) 2009-05-26 13:14:37

Repository: asterisk
Revision: 196826

U   branches/1.4/res/res_convert.c

------------------------------------------------------------------------
r196826 | russell | 2009-05-26 13:14:37 -0500 (Tue, 26 May 2009) | 9 lines

Resolve a file handle leak.

The frames here should have always been freed.  However, out of luck, there was
never any memory leaked.  However, after file streams became reference counted,
this code would leak the file stream for the file being read.

(closes issue ASTERISK-14190)
Reported by: jkroon

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=196826
By: Digium Subversion (svnbot) 2009-05-26 13:20:59

Repository: asterisk
Revision: 196843

_U  trunk/
U   trunk/res/res_convert.c

------------------------------------------------------------------------
r196843 | russell | 2009-05-26 13:20:59 -0500 (Tue, 26 May 2009) | 16 lines

Merged revisions 196826 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
 r196826 | russell | 2009-05-26 13:14:36 -0500 (Tue, 26 May 2009) | 9 lines
 
 Resolve a file handle leak.
 
 The frames here should have always been freed.  However, out of luck, there was
 never any memory leaked.  However, after file streams became reference counted,
 this code would leak the file stream for the file being read.
 
 (closes issue ASTERISK-14190)
 Reported by: jkroon
........

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=196843
By: Digium Subversion (svnbot) 2009-05-26 13:25:48

Repository: asterisk
Revision: 196865

_U  branches/1.6.0/
U   branches/1.6.0/res/res_convert.c

------------------------------------------------------------------------
r196865 | russell | 2009-05-26 13:25:47 -0500 (Tue, 26 May 2009) | 23 lines

Merged revisions 196843 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

................
 r196843 | russell | 2009-05-26 13:20:57 -0500 (Tue, 26 May 2009) | 16 lines
 
 Merged revisions 196826 via svnmerge from
 https://origsvn.digium.com/svn/asterisk/branches/1.4
 
 ........
   r196826 | russell | 2009-05-26 13:14:36 -0500 (Tue, 26 May 2009) | 9 lines
   
   Resolve a file handle leak.
   
   The frames here should have always been freed.  However, out of luck, there was
   never any memory leaked.  However, after file streams became reference counted,
   this code would leak the file stream for the file being read.
   
   (closes issue ASTERISK-14190)
   Reported by: jkroon
 ........
................

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=196865
By: Digium Subversion (svnbot) 2009-05-26 13:30:05

Repository: asterisk
Revision: 196869

_U  branches/1.6.1/
U   branches/1.6.1/res/res_convert.c

------------------------------------------------------------------------
r196869 | russell | 2009-05-26 13:30:05 -0500 (Tue, 26 May 2009) | 23 lines

Merged revisions 196843 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

................
 r196843 | russell | 2009-05-26 13:20:57 -0500 (Tue, 26 May 2009) | 16 lines
 
 Merged revisions 196826 via svnmerge from
 https://origsvn.digium.com/svn/asterisk/branches/1.4
 
 ........
   r196826 | russell | 2009-05-26 13:14:36 -0500 (Tue, 26 May 2009) | 9 lines
   
   Resolve a file handle leak.
   
   The frames here should have always been freed.  However, out of luck, there was
   never any memory leaked.  However, after file streams became reference counted,
   this code would leak the file stream for the file being read.
   
   (closes issue ASTERISK-14190)
   Reported by: jkroon
 ........
................

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=196869
By: Digium Subversion (svnbot) 2009-05-26 13:36:30

Repository: asterisk
Revision: 196870

_U  branches/1.6.2/
U   branches/1.6.2/res/res_convert.c

------------------------------------------------------------------------
r196870 | russell | 2009-05-26 13:36:29 -0500 (Tue, 26 May 2009) | 23 lines

Merged revisions 196843 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

................
 r196843 | russell | 2009-05-26 13:20:57 -0500 (Tue, 26 May 2009) | 16 lines
 
 Merged revisions 196826 via svnmerge from
 https://origsvn.digium.com/svn/asterisk/branches/1.4
 
 ........
   r196826 | russell | 2009-05-26 13:14:36 -0500 (Tue, 26 May 2009) | 9 lines
   
   Resolve a file handle leak.
   
   The frames here should have always been freed.  However, out of luck, there was
   never any memory leaked.  However, after file streams became reference counted,
   this code would leak the file stream for the file being read.
   
   (closes issue ASTERISK-14190)
   Reported by: jkroon
 ........
................

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=196870