<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/3653/">https://reviewboard.asterisk.org/r/3653/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">> 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.</pre>
<br />
<p>- Alexander</p>
<br />
<p>On June 20th, 2014, 2:06 p.m. UTC, Alexander Traud wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By Alexander Traud.</div>
<p style="color: grey;"><i>Updated June 20, 2014, 2:06 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-18345">ASTERISK-18345</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
Asterisk
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Asterisk 12.3</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>trunk/channels/chan_sip.c <span style="color: grey">(416319)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/3653/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>