<p>Neels Hofmeyr <strong>uploaded patch set #2</strong> to this change.</p><p><a href="https://gerrit.osmocom.org/13277">View Change</a></p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">add caller-owns-msgb variant osmo_sccp_user_sap_down_nofree()<br><br>Add osmo_sccp_user_sap_down_nofree(), which is identical to<br>osmo_sccp_user_sap_down(), but doesn't imply a msgb_free().<br><br>To implement that, sccp_sclc_user_sap_down_nofree() with the same msgb<br>semantics is required.<br><br>Rationale:<br><br>Avoiding msgb leaks is easiest if the caller retains ownership of the msgb.<br>Take this hypothetical chain where leaks are obviously avoided:<br><br> void send()<br> {<br> msg = msgb_alloc();<br> dispatch(msg);<br> msgb_free(msg);<br> }<br><br> void dispatch(msg)<br> {<br> osmo_fsm_inst_dispatch(fi, msg);<br> }<br><br> void fi_on_event(fi, data)<br> {<br> if (socket_is_ok)<br> socket_write((struct msgb*)data);<br> }<br><br> void socket_write(msgb)<br> {<br> if (!ok1)<br> return;<br> if (ok2) {<br> if (!ok3)<br> return;<br> write(sock, msg->data);<br> }<br> }<br><br>However, if the caller passes ownership down to the msgb consumer, things<br>become nightmarishly complex:<br><br> void send()<br> {<br> msg = msgb_alloc();<br> rc = dispatch(msg);<br> /* dispatching event failed? */<br> if (rc)<br> msgb_free(msg);<br> }<br><br> int dispatch(msg)<br> {<br> if (osmo_fsm_inst_dispatch(fi, msg))<br> return -1;<br> if (something_else())<br> return -1; // <-- double free!<br> }<br><br> void fi_on_event(fi, data)<br> {<br> if (socket_is_ok) {<br> socket_write((struct msgb*)data);<br> else<br> /* socket didn't consume? */<br> msgb_free(data);<br> }<br><br> int socket_write(msgb)<br> {<br> if (!ok1)<br> return -1; // <-- leak!<br> if (ok2) {<br> if (!ok3)<br> goto out;<br> write(sock, msg->data);<br> }<br> out:<br> msgb_free(msg);<br> return -2;<br> }<br><br>If any link in this call chain fails to be aware of the importance to return a<br>failed RC or to free a msgb if the chain is broken, or to not return a failed<br>RC if the msgb is consumed, we have a hidden msgb leak or double free.<br><br>This is the case with osmo_sccp_user_sap_down(). In new osmo-msc, passing data<br>through various FSM instances, there is high potential for leak/double-free<br>bugs. A very large brain is required to track down every msgb path.<br><br>osmo_sccp_user_sap_down_nofree() makes this problem trivial to solve even for<br>humans.<br><br>Change-Id: Ic818efa78b90f727e1a94c18b60d9a306644f340<br>---<br>M include/osmocom/sigtran/sccp_sap.h<br>M src/sccp_internal.h<br>M src/sccp_sclc.c<br>M src/sccp_scoc.c<br>4 files changed, 38 insertions(+), 23 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">git pull ssh://gerrit.osmocom.org:29418/libosmo-sccp refs/changes/77/13277/2</pre><p>To view, visit <a href="https://gerrit.osmocom.org/13277">change 13277</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.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.osmocom.org/13277"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: libosmo-sccp </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: newpatchset </div>
<div style="display:none"> Gerrit-Change-Id: Ic818efa78b90f727e1a94c18b60d9a306644f340 </div>
<div style="display:none"> Gerrit-Change-Number: 13277 </div>
<div style="display:none"> Gerrit-PatchSet: 2 </div>
<div style="display:none"> Gerrit-Owner: Neels Hofmeyr <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder (1000002) </div>
<div style="display:none"> Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-CC: Harald Welte <laforge@gnumonks.org> </div>