<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/4188/">https://reviewboard.asterisk.org/r/4188/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 18th, 2014, 7:25 a.m. CST, <b>Matt Jordan</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">From http://www.washington.edu/imap/documentation/internal.txt.html:
{quote}
* * * IMPORTANT * * *
Any multi-threaded application should test stream->lock prior to
calling any c-client stream functions. Any attempt to call a
mail_xxx() function while one is already in progress on the same
stream will cause the application to fail in unpredictable ways.
Note that this check is insufficient in a preemptive-scheduling
multi-tasking application due to the possibility of a timing race.
Such applications must be written so that only one process accesses
the stream, or to have a higher level lock.
Since MAIL operations will not finish until they are completed, a
single-tasking application does not have to worry about this problem,
except in the callback invoked from MAIL (e.g. mm_exists(), etc.) in which
case the stream is *always* locked.
{quote}
So, a few things from this:
(1) Based on the description from the UW IMAP documentation, locking vms->lock is sufficient *if* other mail accesses also lock the same vms object for that stream. If that isn't happening, then your patch probably won't fix anything, since both mail_open calls were already protected by the vms->lock and your global lock doesn't protect any other mail_XXXX accesses. The only way your patch would affect anything is if there were two different vms objects being used to open the same stream at the same time.
(2) Your backtrace on the issue doesn't show concurrent accesses to vm_execmain - although a lot of symbols are missing - so right now it's impossible to tell who the offending accesses are. It's also impossible to know if it is due to two concurrent accesses of mail_open with different vms objects, or something else.
Generally, I'm not this patch fixes your issue.</pre>
</blockquote>
<p>On November 21st, 2014, 4:03 a.m. CST, <b>Ben Smithurst</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">The concern we found was that some code called by mail_open uses static variables, specifically the ip_nametoaddr function - at least the ip6_unix.c implementation of it, which is where we're seeing crashes. We believe the problem may be 2 DIFFERENT streams are being opened at once, correctly locked individually, but ultimately causing a conflict with a static variable deep inside the uw-imap code.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Thanks for pointing out the issue in the IMAP library.
Apparently UW IMAP decided that the implementation of sockaddr needed help:
* There is some amazingly bad design in IPv6 sockets.
*
* Supposedly, the new getnameinfo() and getaddrinfo() functions create an
* abstraction that is not dependent upon IPv4 or IPv6. However, the
* definition of getnameinfo() requires that the caller pass the length of
* the sockaddr instead of deriving it from sa_family. The man page says
* that there's an sa_len member in the sockaddr, but actually there isn't.
* This means that any caller to getnameinfo() and getaddrinfo() has to know
* the size for the protocol family used by that sockaddr.
*
* The new sockaddr_in6 is bigger than the generic sockaddr (which is what
* connect(), accept(), bind(), getpeername(), getsockname(), etc. expect).
* Rather than increase the size of sockaddr, there's a new sockaddr_storage
* which is only usable for allocating space.
*/
Fantasic.
You're correct that they use static variables in ip_nametoaddr. It certainly has no hope of being thread safe. Given the status of the UW IMAP library, I would suspect we will have to work around any issues in the library.
Looking at the UW IMAP source, ip_nametoaddr is called on opening of a TCP client connection in tcp_open, which is called from mail_open. Grepping the source for other locations that call this function, we find that it is also called from tcp_canonical and tcp_isclienthost. These in turn are called as such:
tcp_canonical:
-- called from c-client/mail.c:mail_usable_network_stream
-- called from c-client/pop3.c:pop3_status
-- called from c-client/mail.c:mail_open_work
-- called from c-client/imap4r1.c:imap_status
-- called from c-client/nntp.c:nntp_status
Of these, the only one that we would call indirectly is mail_open_work through mail_open - which is already protected by your mutex. We currently don't call imap_status - but if we did, it would also need to be protected by the same mutex.
tcp_isclienthost:
-- called from env_unix.c:dorc when set plaintext-allowed-clients appears in the .rc file, which is only called on server_init/env_init (and should only happen once). Given that, there doesn't seem to be any reason to worry about this call.
This patch should be sufficient.
</pre>
<br />
<p>- Matt</p>
<br />
<p>On November 17th, 2014, 9:03 a.m. CST, David Duncan Ross Palmer 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 David Duncan Ross Palmer.</div>
<p style="color: grey;"><i>Updated Nov. 17, 2014, 9:03 a.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-24516">ASTERISK-24516</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;">Asterisk segfaults when playing back voicemail under high concurrency with an IMAP backend</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;">Rapid load testing performed with {{SIPp}}:
{code}
sipp -sn uac -d 7000 -s ‘*1’ 127.0.0.1 -l 400 -mp 5606
{code}</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/apps/app_voicemail.c <span style="color: grey">(427675)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/4188/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>