[asterisk-dev] [Code Review] Change cleanup ordering in filestream destructor.

[Leif Madsen]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/2286/#review7712
-----------------------------------------------------------


I can confirm that the patch resolved a problem on our production servers where we experienced the problem of a zero-length header being written to the recorded file. From the users point of view, they were unable to playback recorded files on applications such as Windows Media Player (although worked fine in VLC).

- Leif


On Jan. 23, 2013, 3:59 p.m., Russell Bryant wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2286/
> -----------------------------------------------------------
> 
> (Updated Jan. 23, 2013, 3:59 p.m.)
> 
> 
> Review request for Asterisk Developers and Leif Madsen.
> 
> 
> Summary
> -------
> 
> This patch came about due to a problem observed where wav files had an
> empty header.  The header is supposed to be updated in wav_close().  It
> turns out that this was broken when the cache_record_files option from
> asterisk.conf was enabled.  The cleanup code was moving the file to its
> final destination *before* running the close() method of the file
> destructor, so the header didn't get updated.
> 
> Another problem here is that the move was being done before actually
> closing the FILE *.
> 
> Finally, the last bug fixed here is that I noticed that wav_close()
> checks for stream->filename to be non-NULL.  In the previous cleanup
> order, it's checking a pointer to freed memory.  This doesn't actually
> cause anything to break, but it's treading on dangerous waters.  Now the
> free() of stream->filename is happening after the format module's
> close() method gets called, so it's safer.
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/main/file.c 380022 
> 
> Diff: https://reviewboard.asterisk.org/r/2286/diff
> 
> 
> Testing
> -------
> 
> Problem reported by Leif Madsen.  He confirmed that this patch fixed his issue.
> 
> 
> Thanks,
> 
> Russell
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130123/b96e79b7/attachment.htm>