[asterisk-dev] [Code Review] 3481: Websocket: Add locking around session access and modification

[opticron]

> On April 25, 2014, 2:04 p.m., Matt Jordan wrote:
> > branches/11/res/res_http_websocket.c, lines 526-530
> > <https://reviewboard.asterisk.org/r/3481/diff/1/?file=57910#file57910line526>
> >
> >     So, while locking may solve the issue, there's something more insidious about this part of the code that doesn't sit well with me. (Note: this is the actual culprit that causes a crash in the websocket write)
> >     
> >     I'm not sure that the way this is currently handled is the right way to handle a AST_WEBSOCKET_OPCODE_CLOSE. The session destructor will already close the the file descriptor. Ideally, we'd just let the destruction of the session do this work for us.
> >     
> >     It feels like the right way to handle this may be to just let the caller of ast_websocket_read know that they were told that the session needs to die. That would let them de-ref the session appropriately. If a concurrent write was occurring at the same time, when the write completes, the session would be terminated.
> >     
> >     Now, whether or not it's allowed to have a write complete when you've just been told to close the websocket is another question. If not, then we have to keep all of the locking in here.

There is provision for this situation in the RFC stating that the side of the connection wishing to close the connection must wait for a close acknowledgement to shut down the TCP connection.

I'd like to keep the locking in place for ast_websocket_close and ast_websocket_write as there is a chance of interleaved writes since the header and payload in ast_websocket_write are written in two different fwrite()s.


- opticron


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


On April 25, 2014, 1:46 p.m., opticron wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3481/
> -----------------------------------------------------------
> 
> (Updated April 25, 2014, 1:46 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-23605
>     https://issues.asterisk.org/jira/browse/ASTERISK-23605
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This resolves a race condition where data could be written to a NULL FILE pointer causing a crash as a websocket connection was in the process of shutting down by adding locking to accesses and modifications of the websocket session struct.
> 
> 
> Diffs
> -----
> 
>   branches/11/res/res_http_websocket.c 413007 
> 
> Diff: https://reviewboard.asterisk.org/r/3481/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> opticron
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140428/82d0dcb1/attachment.html>