[Asterisk-code-review] res http websocket.c: prevent avoidable disconnections cause... (asterisk[11])

[Matt Jordan]

Matt Jordan has posted comments on this change.

Change subject: res_http_websocket.c: prevent avoidable disconnections caused by write errors
......................................................................


Patch Set 2: Code-Review+1

> Hi Matt,
 > Thank you for the feedback, I've corrected the code standard
 > violations in the second patch set and will pay more attention to
 > these in the future.
 > 
 > Regarding the close due to !session->f, I've removed that entire
 > control block from the second patch set - I forgot that we'd added
 > that assertion in separately from the changes referenced in the
 > commit notes I submitted.  We saw a couple core dumps a long time
 > ago with SIGSEGV due to null pointer dereference on session->f in
 > this part of the code, but to correct it by checking for session->f
 > here & by rewriting ast_websocket_close, would only serve to mask
 > the source of the issue (and I've seen past reviews where to do so
 > was advised against), so I'll keep it as an internal patch &
 > hopefully someone will open an issue with a relevant backtrace so
 > that the source of the null pointer can be identified, if it is
 > still a problem.  I'm surprised that we didn't start seeing crashes
 > in ast_websocket_close after patching that, so maybe someone else
 > patched the source of that problem around the same time as us
 > dealing with it in ast_websocket_write.

At first when I looked at it, I thought adding the NULL check was advisable. Upon further investigation, I'm not sure how we'd be getting into that state. While ast_websocket_close does set session->f to NULL, it does so only:

* When it has safely locked the session
* After it has marked the session as closing

Conversely, ast_websocket_write does not attempt to use session->f until after it has:

* Locked the session
* Checked the closing flag to make sure that we aren't in a closed state

Since the closing predicate appears to be honoured, I'm not sure where we would end up in a state where we aren't closing, but don't have a valid file stream set up on the session. So I'm with you - I'd rather make sure I understand what is occurring. Otherwise, we may put that NULL check in there, and merely move the problem to some other location.

-- 
To view, visit https://gerrit.asterisk.org/1863
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4db7a586af1c7a57184c31d3d55bf146f1a40598
Gerrit-PatchSet: 2
Gerrit-Project: asterisk
Gerrit-Branch: 11
Gerrit-Owner: Dade Brandon <dade at xencall.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Dade Brandon <dade at xencall.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-HasComments: No