<p> Attention is currently required from: Kevin Harwell. </p>
<p><a href="https://gerrit.asterisk.org/c/asterisk/+/18529">View Change</a></p><p>3 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="null">File apps/app_festival.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18529/comment/6861292d_6b2298fd">Patch Set #1, Line 436:</a> <code style="font-family:monospace,monospace"> if (strlen(cachedir) + sizeof(MD5Hex) + 1 <= MAXFESTLEN && (usecache == -1)) {</code></p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">Not sure this is the right fix here? Two possible problems:</p><p style="white-space: pre-wrap; word-wrap: break-word;">1) sizeof(x) is not always equal to strlen(x)</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">In this case, MD5Hex is declared as char[33] so the sizeof allows the compiler to know the size in advance. Apparently the compiler isn't smart enough to realize that the buffer is still fixed at 33 even using strlen.</p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">2) with sizeof I think you can remove the "+ 1" since sizeof returns the full size of the "array" whereas strlen returns the length of the string -1 for NULL char.</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">We still have to account for the '/'. The original should have used 2 to account for the NULL and the '/' so it was wrong to begin with.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File channels/chan_sip.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18529/comment/baf54726_dd813d3c">Patch Set #1, Line 35425:</a> <code style="font-family:monospace,monospace"> if ((void *)(expected_start) != (void *)start) { \</code></p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">Curious what was the problem here? Comparison to NULL needs a cast now?</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">Actually, the change in line 35425 isn't technically needed but I played it safe. The issue in line 35426 is that most callers of the CHECK_RESULTS pass in a value like "input + 8" and even though "input" is a "char *", if you add a const number to it, it can never be 0/NULL and will always evaluate to "true". That's what the compiler was complaining about in the original ternary condition. Just changing the ternary condition from (expected_start) to (expected_start != NULL) doesn't help because the compiler knows that NULL is "0" so it still complains. You have to hide the fact that NULL is "0" from the compiler by casting it to "void *". It's stupid I know but that's the way it is right now.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File include/asterisk/strings.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18529/comment/5dd80de8_2e668150">Patch Set #1, Line 396:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> volatile size_t sz = size;<br> volatile char *sp = (char *)src;<br></pre></blockquote></p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">I _think_ this breaks ABI as this is an inline function defined in a header, so altering stuff here could change the size of the resulting library.</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">If a module has inlined it, the code is in the module so changing it here has no effect on the module until the module recompiles. At that point it'll pick up the new version. Yeah?</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/18529">change 18529</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/c/asterisk/+/18529"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 16 </div>
<div style="display:none"> Gerrit-Change-Id: Ia081ca1bcfb329df6487c4660aaf1944309eb570 </div>
<div style="display:none"> Gerrit-Change-Number: 18529 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </div>
<div style="display:none"> Gerrit-Reviewer: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Michael Bradeen <mbradeen@sangoma.com> </div>
<div style="display:none"> Gerrit-Attention: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Thu, 05 May 2022 12:15:06 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Comment-In-Reply-To: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>