> On June 27, 2014, 9:42 a.m., Matt Jordan wrote: > > > > Alexander Traud wrote: > > hence a 'retry once' poll may not be sufficient regardless to read all > of the data from the socket. > > I am not sure, I understand you guys. Just to clarify my intentions: The > proposed patch is not about to fix/resolve all issues in TLS reading. > Actually, I think the latest changes in tcptls.c do this already. However > here, this patch tries to workaround one bug in the existing code. Our > current code does: > 1. ast_wait_for_input (no operation, from my point of view) > 2. fgets > 3. (optionally) ast_wait_for_input > 4. (optionally) fgets > … and so on. > > Here, the proposed patch changes this to > 1. fgets > 2. (optionally) ast_wait_for_input > 3. (optionally) fgets > 4. (optionally) ast_wait_for_input > … and so on. > > The released code (as of Asterisk 12.3.2) fails in my corner case (see > the appended bug), because the underlying SSL_read returned > SSL_ERROR_WANT_READ. Therefore in step 2, fgets returned -1 already, > therefore the while loop is exited with the failure code -1. > > Yes, theoretically, fgets could return -1 more than once. Therefore a > retry-once *might* not be sufficient. However, this is not what this patch is > about to fix. I do not face that particular issue (retry-n required), nor do > I try to solve that. Until someone offers a patch which introduces a > retry-n-times, I would like to see this patch to pass.
I agree that the current situation is not good for large packets coming across a TLS connection. Generally, however, when we fix a problem, we really do try to fix it for good. Sometimes we don't succeed - but the goal is usually to make sure that when an issue gets closed, it stays closed for good. I'll grant that there are times this rule gets bent - particularly when there are major restructuring issues that would have to occur to fix a problem. Sometimes, then, a band-aid is sufficient. That probably was the case here once upon a time, however, several security vulnerabilities later, and the vast majority of the restructuring has already been done. The TCP/TLS structure supports the concept of overflow; the underlying tcptls layer has been structured better to handle the reading; the chan_sip code needs to be updated. I don't think this patch resolves the issue. It may fix it for some people - maybe even most people - but all that means is that someone else will run into it again, and the problem is now more insidious: we "fixed it", without actually fixing it. Now they come to the bug report, it's closed, and they open up another bug report. More bug marshalling ensues, trying to understand how we fixed it and its still broken. Or we leave the bug report open, and we magically "fix" it for some people without them knowing. Neither solution is good. If we're going to make it so that reading large packets over a TLS socket works, I'd prefer to do it correctly, or not at all. - Matt ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3653/#review12371 ----------------------------------------------------------- On June 20, 2014, 9:06 a.m., Alexander Traud wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/3653/ > ----------------------------------------------------------- > > (Updated June 20, 2014, 9:06 a.m.) > > > Review request for Asterisk Developers. > > > Bugs: ASTERISK-18345 > https://issues.asterisk.org/jira/browse/ASTERISK-18345 > > > Repository: Asterisk > > > Description > ------- > > With some large SDP, a *second* poll is required on the first part of a TLS > message. > > The current code did not poll a second time because the variable need_poll > was inited with yes (1). That poll was a no-operation because there was a > socket event already (which mandates fgets without poll). In the current > code, poll returned immediately, fgets returned NULL, after_poll was yes (1), > sip_tls_read returned failed (-1), _sip_tcp_helper_thread went to cleanup, > called ast_tcptls_close_session_file, which closed the TLS connection. > > The proposed patch, reads the gets the first message. If that failed, it does > poll. This fixed all large SDP issues with SIP over TLS which I faced. > > I am aware there were changes committed to tcptls.c just recently (revision > 415907). Anyway, let us fix this bug as well. > > > Diffs > ----- > > trunk/channels/chan_sip.c 416319 > > Diff: https://reviewboard.asterisk.org/r/3653/diff/ > > > Testing > ------- > > Asterisk 12.3 > > > Thanks, > > Alexander Traud > >
-- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
