[asterisk-dev] [Code Review] Fix fd leak when using HTTP AMI sessions

[Kevin Fleming]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/148/#review376
-----------------------------------------------------------

Ship it!


With these changes, this looks ready to go! Nice work.


/branches/1.6.0/main/manager.c
<http://reviewboard.digium.com/r/148/#comment896>

    Please explicitly initialize 's' here; " = { .session = NULL }" should be adequate.



/branches/1.6.0/main/manager.c
<http://reviewboard.digium.com/r/148/#comment897>

    Same initialization comment as in session_do().


- Kevin


On 2009-02-10 13:50:13, Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/148/
> -----------------------------------------------------------
> 
> (Updated 2009-02-10 13:50:13)
> 
> 
> Review request for Asterisk Developers, Kevin Fleming and Ryan Brindley.
> 
> 
> Summary
> -------
> 
> Currently there is an fd leak when using http manager sessions with Asterisk. When using the Asterisk-GUI, this can lead to the GUI becoming completely non-functional in a matter of about 24 hours.
> 
> A mansession struct is used in manager.c to represent a single manager session. Each mansession has a f and fd field representing a FILE * and its underlying file descriptor. For typical Manager sessions, these represent the TCP socket over which we are communicating. For http Manager sessions, however, each Manager action leads to the creation and destruction of a new f and fd. Because of a lack of thread synchronization, It is possible for a second manager action to overwrite the f and fd fields being used during a first manager action without the file in use being properly closed. Since the Asterisk GUI fires off several commands at once, this happens frequently when using it.
> 
> The fix that is being used here will seem confusing at first, but hopefully the large block of comments I added near the top of the file will help to explain things more clearly. I consider the review here to be ugly, but it's nice in that it preserves the Manager API and fixes the problem.
> 
> 
> Diffs
> -----
> 
>   /branches/1.6.0/main/manager.c 174327 
> 
> Diff: http://reviewboard.digium.com/r/148/diff
> 
> 
> Testing
> -------
> 
> I started GUI sessions and made sure that pages refreshed quickly. I also checked to be sure that no fd's were leaking when doing the tests.
> 
> 
> Thanks,
> 
> Mark
> 
>