<p>lynxis lazus <strong>submitted</strong> this change.</p><p><a href="https://gerrit.osmocom.org/c/libosmocore/+/25147">View Change</a></p><div style="white-space:pre-wrap">Approvals:
  Jenkins Builder: Verified
  laforge: Looks good to me, approved

</div><pre style="font-family: monospace,monospace; white-space: pre-wrap;">gprs_ns2: don't use llist_for_each when freeing an element<br><br>The problem are recursive execution because a free generates an event which could<br>allow the use to free a nsvcs while the llist_for_each() is still running.<br><br>Change-Id: I902557fb6e56e6588728a46e43a9cbe3215d5c68<br>---<br>M src/gb/gprs_ns2.c<br>M src/gb/gprs_ns2_internal.h<br>M src/gb/gprs_ns2_sns.c<br>3 files changed, 28 insertions(+), 20 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><span>diff --git a/src/gb/gprs_ns2.c b/src/gb/gprs_ns2.c</span><br><span>index 326312c..a895e3d 100644</span><br><span>--- a/src/gb/gprs_ns2.c</span><br><span>+++ b/src/gb/gprs_ns2.c</span><br><span>@@ -664,22 +664,29 @@</span><br><span>       talloc_free(nsvc);</span><br><span> }</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+void ns2_free_nsvcs(struct gprs_ns2_nse *nse)</span><br><span style="color: hsl(120, 100%, 40%);">+{</span><br><span style="color: hsl(120, 100%, 40%);">+ struct gprs_ns2_vc *nsvc;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+   /* prevent recursive free() when the user reacts on a down event and free() a second time */</span><br><span style="color: hsl(120, 100%, 40%);">+  while (!llist_empty(&nse->nsvc)) {</span><br><span style="color: hsl(120, 100%, 40%);">+             nsvc = llist_first_entry(&nse->nsvc, struct gprs_ns2_vc, list);</span><br><span style="color: hsl(120, 100%, 40%);">+                gprs_ns2_free_nsvc(nsvc);</span><br><span style="color: hsl(120, 100%, 40%);">+     }</span><br><span style="color: hsl(120, 100%, 40%);">+}</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> /*! Destroy/release all NS-VC of given NSE</span><br><span>  *  \param[in] nse NSE</span><br><span>  */</span><br><span> void gprs_ns2_free_nsvcs(struct gprs_ns2_nse *nse)</span><br><span> {</span><br><span style="color: hsl(0, 100%, 40%);">-        struct gprs_ns2_vc *nsvc, *tmp;</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span>      if (!nse || nse->freed)</span><br><span>           return;</span><br><span> </span><br><span>  if (nse->bss_sns_fi) {</span><br><span>            osmo_fsm_inst_dispatch(nse->bss_sns_fi, NS2_SNS_EV_REQ_FREE_NSVCS, NULL);</span><br><span>         } else {</span><br><span style="color: hsl(0, 100%, 40%);">-                llist_for_each_entry_safe(nsvc, tmp, &nse->nsvc, list) {</span><br><span style="color: hsl(0, 100%, 40%);">-                 gprs_ns2_free_nsvc(nsvc);</span><br><span style="color: hsl(0, 100%, 40%);">-               }</span><br><span style="color: hsl(120, 100%, 40%);">+             ns2_free_nsvcs(nse);</span><br><span>         }</span><br><span> }</span><br><span> </span><br><span>@@ -893,7 +900,6 @@</span><br><span>  *  \param[in] nse NS Entity to destroy */</span><br><span> void gprs_ns2_free_nse(struct gprs_ns2_nse *nse)</span><br><span> {</span><br><span style="color: hsl(0, 100%, 40%);">- struct gprs_ns2_vc *nsvc, *nsvc2;</span><br><span>    if (!nse || nse->freed)</span><br><span>           return;</span><br><span> </span><br><span>@@ -907,9 +913,7 @@</span><br><span>    gprs_ns2_free_nsvcs(nse);</span><br><span>    ns2_prim_status_ind(nse, NULL, 0, GPRS_NS2_AFF_CAUSE_FAILURE);</span><br><span>       rate_ctr_group_free(nse->ctrg);</span><br><span style="color: hsl(0, 100%, 40%);">-      llist_for_each_entry_safe(nsvc, nsvc2, &nse->nsvc, list) {</span><br><span style="color: hsl(0, 100%, 40%);">-               gprs_ns2_free_nsvc(nsvc);</span><br><span style="color: hsl(0, 100%, 40%);">-       }</span><br><span style="color: hsl(120, 100%, 40%);">+     ns2_free_nsvcs(nse);</span><br><span> </span><br><span>     llist_del(&nse->list);</span><br><span>        talloc_free(nse);</span><br><span>@@ -917,9 +921,11 @@</span><br><span> </span><br><span> void gprs_ns2_free_nses(struct gprs_ns2_inst *nsi)</span><br><span> {</span><br><span style="color: hsl(0, 100%, 40%);">- struct gprs_ns2_nse *nse, *ntmp;</span><br><span style="color: hsl(120, 100%, 40%);">+      struct gprs_ns2_nse *nse;</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">-   llist_for_each_entry_safe(nse, ntmp, &nsi->nse, list) {</span><br><span style="color: hsl(120, 100%, 40%);">+        /* prevent recursive free() when the user reacts on a down event and free() a second time */</span><br><span style="color: hsl(120, 100%, 40%);">+  while (!llist_empty(&nsi->nse)) {</span><br><span style="color: hsl(120, 100%, 40%);">+              nse = llist_first_entry(&nsi->nse, struct gprs_ns2_nse, list);</span><br><span>                gprs_ns2_free_nse(nse);</span><br><span>      }</span><br><span> }</span><br><span>@@ -1473,19 +1479,21 @@</span><br><span>  *  \param[in] bind the bind we want to destroy */</span><br><span> void gprs_ns2_free_bind(struct gprs_ns2_vc_bind *bind)</span><br><span> {</span><br><span style="color: hsl(0, 100%, 40%);">-   struct gprs_ns2_vc *nsvc, *tmp;</span><br><span style="color: hsl(120, 100%, 40%);">+       struct gprs_ns2_vc *nsvc;</span><br><span>    struct gprs_ns2_nse *nse;</span><br><span>    if (!bind || bind->freed)</span><br><span>                 return;</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span>      bind->freed = true;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span>     if (gprs_ns2_is_ip_bind(bind)) {</span><br><span>             llist_for_each_entry(nse, &bind->nsi->nse, list) {</span><br><span>                         gprs_ns2_sns_del_bind(nse, bind);</span><br><span>            }</span><br><span>    }</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">-   llist_for_each_entry_safe(nsvc, tmp, &bind->nsvc, blist) {</span><br><span style="color: hsl(120, 100%, 40%);">+     /* prevent recursive free() when the user reacts on a down event and free() a second time */</span><br><span style="color: hsl(120, 100%, 40%);">+  while (!llist_empty(&bind->nsvc)) {</span><br><span style="color: hsl(120, 100%, 40%);">+            nsvc = llist_first_entry(&bind->nsvc, struct gprs_ns2_vc, blist);</span><br><span>             gprs_ns2_free_nsvc(nsvc);</span><br><span>    }</span><br><span> </span><br><span>@@ -1500,9 +1508,11 @@</span><br><span> </span><br><span> void gprs_ns2_free_binds(struct gprs_ns2_inst *nsi)</span><br><span> {</span><br><span style="color: hsl(0, 100%, 40%);">-  struct gprs_ns2_vc_bind *bind, *tbind;</span><br><span style="color: hsl(120, 100%, 40%);">+        struct gprs_ns2_vc_bind *bind;</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">-      llist_for_each_entry_safe(bind, tbind, &nsi->binding, list) {</span><br><span style="color: hsl(120, 100%, 40%);">+  /* prevent recursive free() when the user reacts on a down event and free() a second time */</span><br><span style="color: hsl(120, 100%, 40%);">+  while (!llist_empty(&nsi->binding)) {</span><br><span style="color: hsl(120, 100%, 40%);">+          bind = llist_first_entry(&nsi->binding, struct gprs_ns2_vc_bind, list);</span><br><span>               gprs_ns2_free_bind(bind);</span><br><span>    }</span><br><span> }</span><br><span>diff --git a/src/gb/gprs_ns2_internal.h b/src/gb/gprs_ns2_internal.h</span><br><span>index d2407f6..ca6bfb7 100644</span><br><span>--- a/src/gb/gprs_ns2_internal.h</span><br><span>+++ b/src/gb/gprs_ns2_internal.h</span><br><span>@@ -354,6 +354,7 @@</span><br><span>                             enum gprs_ns2_vc_mode vc_mode,</span><br><span>                               const char *id);</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+void ns2_free_nsvcs(struct gprs_ns2_nse *nse);</span><br><span> int ns2_bind_alloc(struct gprs_ns2_inst *nsi, const char *name,</span><br><span>                struct gprs_ns2_vc_bind **result);</span><br><span> </span><br><span>diff --git a/src/gb/gprs_ns2_sns.c b/src/gb/gprs_ns2_sns.c</span><br><span>index e96f6b3..43e4920 100644</span><br><span>--- a/src/gb/gprs_ns2_sns.c</span><br><span>+++ b/src/gb/gprs_ns2_sns.c</span><br><span>@@ -1565,7 +1565,6 @@</span><br><span> {</span><br><span>      struct ns2_sns_state *gss = (struct ns2_sns_state *) fi->priv;</span><br><span>    struct gprs_ns2_nse *nse = nse_inst_from_fi(fi);</span><br><span style="color: hsl(0, 100%, 40%);">-        struct gprs_ns2_vc *nsvc, *nsvc2;</span><br><span> </span><br><span>        /* reset when receiving NS2_SNS_EV_REQ_NO_NSVC */</span><br><span>    switch (event) {</span><br><span>@@ -1581,9 +1580,7 @@</span><br><span>             /* tear down previous state</span><br><span>           * gprs_ns2_free_nsvcs() will trigger NO_NSVC, prevent this from triggering a reselection */</span><br><span>                 gss->reselection_running = true;</span><br><span style="color: hsl(0, 100%, 40%);">-             llist_for_each_entry_safe(nsvc, nsvc2, &nse->nsvc, list) {</span><br><span style="color: hsl(0, 100%, 40%);">-                       gprs_ns2_free_nsvc(nsvc);</span><br><span style="color: hsl(0, 100%, 40%);">-               }</span><br><span style="color: hsl(120, 100%, 40%);">+             ns2_free_nsvcs(nse);</span><br><span>                 ns2_clear_elems(&gss->local);</span><br><span>                 ns2_clear_elems(&gss->remote);</span><br><span> </span><br><span></span><br></pre><p>To view, visit <a href="https://gerrit.osmocom.org/c/libosmocore/+/25147">change 25147</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/c/libosmocore/+/25147"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: libosmocore </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I902557fb6e56e6588728a46e43a9cbe3215d5c68 </div>
<div style="display:none"> Gerrit-Change-Number: 25147 </div>
<div style="display:none"> Gerrit-PatchSet: 6 </div>
<div style="display:none"> Gerrit-Owner: lynxis lazus <lynxis@fe80.eu> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: dexter <pmaier@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Reviewer: lynxis lazus <lynxis@fe80.eu> </div>
<div style="display:none"> Gerrit-CC: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-CC: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-MessageType: merged </div>