<p>Neels Hofmeyr has uploaded this change for <strong>review</strong>.</p><p><a href="https://gerrit.osmocom.org/10638">View Change</a></p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">lchan_fsm: safer 'concluded' flag<br><br>The flag lchan->activate.concluded prevents entering the<br>lchan_on_fully_established()/lchan_on_activation_failure() more than once. So<br>far it was checked by callers, instead place in the functions themselves.<br><br>There is a potential functional change here, since during lchan_fail(), the<br>concluded flag was set only after entering lchan_on_activation_failure(). Now<br>it is already set at the start and prevents multiple re-entry beyond doubt.<br><br>This patch is not a result of an actual observed faulty behavior, just from<br>reading the code and seeing the slight loophole.<br><br>Change-Id: I0c906438b05442d66255203eb816453b7193a6d8<br>---<br>M include/osmocom/bsc/gsm_data.h<br>M src/osmo-bsc/lchan_fsm.c<br>2 files changed, 13 insertions(+), 8 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">git pull ssh://gerrit.osmocom.org:29418/osmo-bsc refs/changes/38/10638/1</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><span>diff --git a/include/osmocom/bsc/gsm_data.h b/include/osmocom/bsc/gsm_data.h</span><br><span>index b827d0a..d6e8954 100644</span><br><span>--- a/include/osmocom/bsc/gsm_data.h</span><br><span>+++ b/include/osmocom/bsc/gsm_data.h</span><br><span>@@ -505,7 +505,10 @@</span><br><span> struct {</span><br><span> enum lchan_activate_mode activ_for;</span><br><span> bool activ_ack; /*< true as soon as RSL Chan Activ Ack is received */</span><br><span style="color: hsl(0, 100%, 40%);">- bool concluded; /*< true as soon as LCHAN_ST_ESTABLISHED is reached */</span><br><span style="color: hsl(120, 100%, 40%);">+ /*! This flag ensures that when an lchan activation has succeeded, and we have already</span><br><span style="color: hsl(120, 100%, 40%);">+ * sent ACKs like Immediate Assignment or BSSMAP Assignment Complete, and if other errors</span><br><span style="color: hsl(120, 100%, 40%);">+ * occur later, e.g. during release, that we don't send a NACK out of context. */</span><br><span style="color: hsl(120, 100%, 40%);">+ bool concluded;</span><br><span> bool requires_voice_stream;</span><br><span> bool wait_before_switching_rtp; /*< true = requires LCHAN_EV_READY_TO_SWITCH_RTP */</span><br><span> uint16_t msc_assigned_cic;</span><br><span>diff --git a/src/osmo-bsc/lchan_fsm.c b/src/osmo-bsc/lchan_fsm.c</span><br><span>index e856fd0..13ae7ad 100644</span><br><span>--- a/src/osmo-bsc/lchan_fsm.c</span><br><span>+++ b/src/osmo-bsc/lchan_fsm.c</span><br><span>@@ -88,6 +88,10 @@</span><br><span> struct gsm_subscriber_connection *for_conn,</span><br><span> const char *file, int line)</span><br><span> {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (lchan->activate.concluded)</span><br><span style="color: hsl(120, 100%, 40%);">+ return;</span><br><span style="color: hsl(120, 100%, 40%);">+ lchan->activate.concluded = true;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> switch (activ_for) {</span><br><span> </span><br><span> case FOR_MS_CHANNEL_REQUEST:</span><br><span>@@ -135,6 +139,10 @@</span><br><span> </span><br><span> static void lchan_on_fully_established(struct gsm_lchan *lchan)</span><br><span> {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (lchan->activate.concluded)</span><br><span style="color: hsl(120, 100%, 40%);">+ return;</span><br><span style="color: hsl(120, 100%, 40%);">+ lchan->activate.concluded = true;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> switch (lchan->activate.activ_for) {</span><br><span> case FOR_MS_CHANNEL_REQUEST:</span><br><span> /* No signalling to do here, MS is free to use the channel, and should go on to connect</span><br><span>@@ -222,9 +230,7 @@</span><br><span> lchan_set_last_error(_lchan, "lchan %s in state %s: " fmt, \</span><br><span> _lchan->activate.concluded ? "failure" : "allocation failed", \</span><br><span> osmo_fsm_state_name(fsm, state_was), ## args); \</span><br><span style="color: hsl(0, 100%, 40%);">- if (!_lchan->activate.concluded) \</span><br><span style="color: hsl(0, 100%, 40%);">- lchan_on_activation_failure(_lchan, _lchan->activate.activ_for, _lchan->conn); \</span><br><span style="color: hsl(0, 100%, 40%);">- _lchan->activate.concluded = true; \</span><br><span style="color: hsl(120, 100%, 40%);">+ lchan_on_activation_failure(_lchan, _lchan->activate.activ_for, _lchan->conn); \</span><br><span> if (fi->state != state_chg) \</span><br><span> lchan_fsm_state_chg(state_chg); \</span><br><span> else \</span><br><span>@@ -751,10 +757,6 @@</span><br><span> return;</span><br><span> }</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- /* This flag ensures that when an lchan activation has succeeded, and we have already sent ACKs</span><br><span style="color: hsl(0, 100%, 40%);">- * like Immediate Assignment or BSSMAP Assignment Complete, and if then, way later, some other</span><br><span style="color: hsl(0, 100%, 40%);">- * error occurs, e.g. during release, that we don't send a NACK out of context. */</span><br><span style="color: hsl(0, 100%, 40%);">- lchan->activate.concluded = true;</span><br><span> lchan_on_fully_established(lchan);</span><br><span> }</span><br><span> </span><br><span></span><br></pre><p>To view, visit <a href="https://gerrit.osmocom.org/10638">change 10638</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/10638"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: osmo-bsc </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: newchange </div>
<div style="display:none"> Gerrit-Change-Id: I0c906438b05442d66255203eb816453b7193a6d8 </div>
<div style="display:none"> Gerrit-Change-Number: 10638 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Neels Hofmeyr <nhofmeyr@sysmocom.de> </div>