<p>Corey Farrell <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/7637">View Change</a></p><p>Patch set 1:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #d4ffd4;">Code-Review +1</span></p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">I prefer the reference stealing form but what makes it safer?</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">The ast_json_pack() uses json_vpack_ex().  When json_vpack_ex()<br>gets its own ref for the 'O' option it uses the internal ref<br>function which is not thread safe.  Though if there happens to be<br>an OOM error packing the json object, it will use the internal<br>unref function which is not thread safe either.  However, the OOM<br>case should be quite rare.  Who runs out of memory. :)</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">The situations where json_vpack_ex does an unref will be expanding in v2.11.</p><p style="white-space: pre-wrap; word-wrap: break-word;">ast_json_pack("{s:o, s:o}", "test1", NULL, "test2", ast_json_ref(blob));</p><p style="white-space: pre-wrap; word-wrap: break-word;">This currently leaks because the processing stops at the NULL argument.  My patch to fix this leak in jansson has been merged so starting with v2.11 passing NULL where an object is expected will not leak blob but it will use the jansson unref.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Your patch does make the success case thread safe so that is good.</p><ul style="list-style: none; padding-left: 20px;"></ul><p>To view, visit <a href="https://gerrit.asterisk.org/7637">change 7637</a>. To unsubscribe, 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/7637"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 13 </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I49204db2e57ae96eee43909c18ed007c09ac817e </div>
<div style="display:none"> Gerrit-Change-Number: 7637 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Richard Mudgett <rmudgett@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Corey Farrell <git@cfware.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 </div>
<div style="display:none"> Gerrit-Reviewer: Richard Mudgett <rmudgett@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 19 Dec 2017 00:29:00 +0000 </div>
<div style="display:none"> Gerrit-HasComments: No </div>