Change in libosmo-sccp[master]: add caller-owns-msgb variant osmo_sccp_user_sap_down2()

Neels Hofmeyr gerrit-no-reply at lists.osmocom.org
Fri Mar 15 14:58:01 UTC 2019


Neels Hofmeyr has uploaded this change for review. ( https://gerrit.osmocom.org/13277


Change subject: add caller-owns-msgb variant osmo_sccp_user_sap_down2()
......................................................................

add caller-owns-msgb variant osmo_sccp_user_sap_down2()

Add osmo_sccp_user_sap_down2(), which is identical to
osmo_sccp_user_sap_down(), but doesn't imply a msgb_free().

To implement that, sccp_sclc_user_sap_down2() with the same msgb semantics is
required.

Rationale:

Avoiding msgb leaks is easiest if the caller retains ownership of the msgb.
Take this hypothetical chain where leaks are obviously avoided:

  void send()
  {
  	msg = msgb_alloc();
	dispatch(msg);
	msgb_free(msg);
  }

  void dispatch(msg)
  {
  	osmo_fsm_inst_dispatch(fi, msg);
  }

  void fi_on_event(fi, data)
  {
	if (socket_is_ok)
		socket_write((struct msgb*)data);
  }

  void socket_write(msgb)
  {
  	if (!ok1)
		return;
	if (ok2) {
		if (!ok3)
			return;
		write(sock, msg->data);
	}
  }

However, if the caller passes ownership down to the msgb consumer, things
become nightmarishly complex:

  void send()
  {
  	msg = msgb_alloc();
	rc = dispatch(msg);
	/* dispatching event failed? */
	if (rc)
		msgb_free(msg);
  }

  int dispatch(msg)
  {
  	if (osmo_fsm_inst_dispatch(fi, msg))
		return -1;
	if (something_else())
		return -1; // <-- double free!
  }

  void fi_on_event(fi, data)
  {
	if (socket_is_ok) {
		socket_write((struct msgb*)data);
	else
		/* socket didn't consume? */
		msgb_free(data);
  }

  int socket_write(msgb)
  {
  	if (!ok1)
		return -1; // <-- leak!
	if (ok2) {
		if (!ok3)
			goto out;
		write(sock, msg->data);
	}
  out:
        msgb_free(msg);
	return -2;
  }

If any link in this call chain fails to be aware of the importance to return a
failed RC or to free a msgb if the chain is broken, or to not return a failed
RC if the msgb is consumed, we have a hidden msgb leak or double free.

This is the case with osmo_sccp_user_sap_down(). In new osmo-msc, passing data
through various FSM instances, there is high potential for leak/double-free
bugs. A very large brain is required to track down every msgb path.

osmo_sccp_user_sap_down2() makes this problem trivial to solve even for humans.

Change-Id: Ic818efa78b90f727e1a94c18b60d9a306644f340
---
M include/osmocom/sigtran/sccp_sap.h
M src/sccp_internal.h
M src/sccp_sclc.c
M src/sccp_scoc.c
4 files changed, 36 insertions(+), 23 deletions(-)



  git pull ssh://gerrit.osmocom.org:29418/libosmo-sccp refs/changes/77/13277/1

diff --git a/include/osmocom/sigtran/sccp_sap.h b/include/osmocom/sigtran/sccp_sap.h
index 84d762c..f1fdd98 100644
--- a/include/osmocom/sigtran/sccp_sap.h
+++ b/include/osmocom/sigtran/sccp_sap.h
@@ -263,6 +263,7 @@
 		    osmo_prim_cb prim_cb, uint16_t ssn);
 
 int osmo_sccp_user_sap_down(struct osmo_sccp_user *scu, struct osmo_prim_hdr *oph);
+int osmo_sccp_user_sap_down2(struct osmo_sccp_user *scu, struct osmo_prim_hdr *oph);
 
 struct osmo_ss7_instance *
 osmo_sccp_addr_by_name(struct osmo_sccp_addr *dest_addr,
diff --git a/src/sccp_internal.h b/src/sccp_internal.h
index 000f0f7..f4c723f 100644
--- a/src/sccp_internal.h
+++ b/src/sccp_internal.h
@@ -116,6 +116,7 @@
 
 /* SCU -> SCLC */
 int sccp_sclc_user_sap_down(struct osmo_sccp_user *scu, struct osmo_prim_hdr *oph);
+int sccp_sclc_user_sap_down2(struct osmo_sccp_user *scu, struct osmo_prim_hdr *oph);
 
 struct msgb *sccp_msgb_alloc(const char *name);
 
diff --git a/src/sccp_sclc.c b/src/sccp_sclc.c
index db67660..bfaaafc 100644
--- a/src/sccp_sclc.c
+++ b/src/sccp_sclc.c
@@ -115,15 +115,14 @@
 	return rc;
 }
 
-/*! \brief Main entrance function for primitives from SCCP User
+/*! Main entrance function for primitives from SCCP User
+ * The caller is required to free oph->msg.
  *  \param[in] scu SCCP User who is sending the primitive
  *  \param[on] oph Osmocom primitive header of the primitive
  *  \returns 0 on success; negtive in case of error */
-int sccp_sclc_user_sap_down(struct osmo_sccp_user *scu, struct osmo_prim_hdr *oph)
+int sccp_sclc_user_sap_down2(struct osmo_sccp_user *scu, struct osmo_prim_hdr *oph)
 {
 	struct osmo_scu_prim *prim = (struct osmo_scu_prim *) oph;
-	struct msgb *msg = prim->oph.msg;
-	int rc = 0;
 
 	/* we get called from osmo_sccp_user_sap_down() which already
 	 * has debug-logged the primitive */
@@ -131,20 +130,25 @@
 	switch (OSMO_PRIM_HDR(&prim->oph)) {
 	case OSMO_PRIM(OSMO_SCU_PRIM_N_UNITDATA, PRIM_OP_REQUEST):
 		/* Connectionless by-passes this altogether */
-		rc = xua_gen_encode_and_send(scu, -1, prim, SUA_CL_CLDT);
-		goto out;
+		return xua_gen_encode_and_send(scu, -1, prim, SUA_CL_CLDT);
 	default:
 		LOGP(DLSCCP, LOGL_ERROR, "Received unknown SCCP User "
 		     "primitive %s from user\n",
 		     osmo_scu_prim_name(&prim->oph));
-		rc = -1;
-		goto out;
+		return -1;
 	}
+}
 
-out:
-	/* the SAP is supposed to consume the primitive/msgb */
+/*! Same as sccp_sclc_user_sap_down2() but implies a msgb_free(oph->msg).
+ *  \param[in] scu SCCP User who is sending the primitive
+ *  \param[on] oph Osmocom primitive header of the primitive
+ *  \returns 0 on success; negtive in case of error */
+int sccp_sclc_user_sap_down(struct osmo_sccp_user *scu, struct osmo_prim_hdr *oph)
+{
+	struct osmo_scu_prim *prim = (struct osmo_scu_prim *) oph;
+	struct msgb *msg = prim->oph.msg;
+	int rc = sccp_sclc_user_sap_down2(scu, oph);
 	msgb_free(msg);
-
 	return rc;
 }
 
diff --git a/src/sccp_scoc.c b/src/sccp_scoc.c
index cb1d567..6a56692 100644
--- a/src/sccp_scoc.c
+++ b/src/sccp_scoc.c
@@ -1687,15 +1687,15 @@
 	}
 }
 
-/*! \brief Main entrance function for primitives from SCCP User
+/*! Main entrance function for primitives from SCCP User.
+ * The caller is required to free oph->msg.
  *  \param[in] scu SCCP User sending us the primitive
  *  \param[in] oph Osmocom primitive sent by the user
  *  \returns 0 on success; negative on error */
-int osmo_sccp_user_sap_down(struct osmo_sccp_user *scu, struct osmo_prim_hdr *oph)
+int osmo_sccp_user_sap_down2(struct osmo_sccp_user *scu, struct osmo_prim_hdr *oph)
 {
 	struct osmo_scu_prim *prim = (struct osmo_scu_prim *) oph;
 	struct osmo_sccp_instance *inst = scu->inst;
-	struct msgb *msg = prim->oph.msg;
 	struct sccp_connection *conn;
 	int rc = 0;
 	int event;
@@ -1707,7 +1707,7 @@
 	case OSMO_PRIM(OSMO_SCU_PRIM_N_UNITDATA, PRIM_OP_REQUEST):
 	/* other CL primitives? */
 		/* Connectionless by-passes this altogether */
-		return sccp_sclc_user_sap_down(scu, oph);
+		return sccp_sclc_user_sap_down2(scu, oph);
 	case OSMO_PRIM(OSMO_SCU_PRIM_N_CONNECT, PRIM_OP_REQUEST):
 		/* Allocate new connection structure */
 		conn = conn_create_id(inst, prim->u.connect.conn_id);
@@ -1715,7 +1715,7 @@
 			/* FIXME: inform SCCP user with proper reply */
 			LOGP(DLSCCP, LOGL_ERROR, "Cannot create conn-id for primitive %s\n",
 			     osmo_scu_prim_name(&prim->oph));
-			goto out;
+			return rc;
 		}
 		conn->user = scu;
 		break;
@@ -1729,25 +1729,32 @@
 			/* FIXME: inform SCCP user with proper reply */
 			LOGP(DLSCCP, LOGL_ERROR, "Received unknown conn-id %u for primitive %s\n",
 			     scu_prim_conn_id(prim), osmo_scu_prim_name(&prim->oph));
-			goto out;
+			return rc;
 		}
 		break;
 	default:
 		LOGP(DLSCCP, LOGL_ERROR, "Received unknown primitive %s\n",
 			osmo_scu_prim_name(&prim->oph));
-		rc = -1;
-		goto out;
+		return -1;
 	}
 
 	/* Map from primitive to event */
 	event = osmo_event_for_prim(oph, scu_scoc_event_map);
 
 	/* Dispatch event into connection */
-	rc = osmo_fsm_inst_dispatch(conn->fi, event, prim);
-out:
-	/* the SAP is supposed to consume the primitive/msgb */
-	msgb_free(msg);
+	return osmo_fsm_inst_dispatch(conn->fi, event, prim);
+}
 
+/*! Same as osmo_sccp_user_sap_down2() but implies a msgb_free(oph->msg).
+ *  \param[in] scu SCCP User sending us the primitive
+ *  \param[in] oph Osmocom primitive sent by the user
+ *  \returns 0 on success; negative on error */
+int osmo_sccp_user_sap_down(struct osmo_sccp_user *scu, struct osmo_prim_hdr *oph)
+{
+	struct osmo_scu_prim *prim = (struct osmo_scu_prim *) oph;
+	struct msgb *msg = prim->oph.msg;
+	int rc = osmo_sccp_user_sap_down2(scu, oph);
+	msgb_free(msg);
 	return rc;
 }
 

-- 
To view, visit https://gerrit.osmocom.org/13277
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: libosmo-sccp
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic818efa78b90f727e1a94c18b60d9a306644f340
Gerrit-Change-Number: 13277
Gerrit-PatchSet: 1
Gerrit-Owner: Neels Hofmeyr <nhofmeyr at sysmocom.de>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190315/289bf19d/attachment.html>


More information about the gerrit-log mailing list