summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorNeels Hofmeyr <neels@hofmeyr.de>2019-03-10 04:41:27 +0100
committerNeels Hofmeyr <neels@hofmeyr.de>2019-04-12 06:27:10 +0200
commitac729eb5a8295d0c1fc2dea3fb0f56413654c13e (patch)
treebb8968730c096a2889fefb536842f731ad5bb4e9
parent49eea32ab4158b2b80a3862513363f237fb49939 (diff)
add caller-owns-msgb variant osmo_sccp_user_sap_down_nofree()
Add osmo_sccp_user_sap_down_nofree(), which is identical to osmo_sccp_user_sap_down(), but doesn't imply a msgb_free(). To implement that, sccp_sclc_user_sap_down_nofree() 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_down_nofree() makes this problem trivial to solve even for humans. Change-Id: Ic818efa78b90f727e1a94c18b60d9a306644f340
-rw-r--r--include/osmocom/sigtran/sccp_sap.h1
-rw-r--r--src/sccp_internal.h1
-rw-r--r--src/sccp_sclc.c27
-rw-r--r--src/sccp_scoc.c32
4 files changed, 38 insertions, 23 deletions
diff --git a/include/osmocom/sigtran/sccp_sap.h b/include/osmocom/sigtran/sccp_sap.h
index 84d762c..f64b7aa 100644
--- a/include/osmocom/sigtran/sccp_sap.h
+++ b/include/osmocom/sigtran/sccp_sap.h
@@ -263,6 +263,7 @@ osmo_sccp_user_bind(struct osmo_sccp_instance *inst, const char *name,
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_down_nofree(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 ad8a6fd..8df6c9b 100644
--- a/src/sccp_internal.h
+++ b/src/sccp_internal.h
@@ -116,6 +116,7 @@ int sccp_user_prim_up(struct osmo_sccp_user *scut, struct osmo_scu_prim *prim);
/* SCU -> SCLC */
int sccp_sclc_user_sap_down(struct osmo_sccp_user *scu, struct osmo_prim_hdr *oph);
+int sccp_sclc_user_sap_down_nofree(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..218fb56 100644
--- a/src/sccp_sclc.c
+++ b/src/sccp_sclc.c
@@ -115,15 +115,14 @@ static int xua_gen_encode_and_send(struct osmo_sccp_user *scu, uint32_t event,
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, otherwise the same as sccp_sclc_user_sap_down().
* \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_down_nofree(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,26 @@ int sccp_sclc_user_sap_down(struct osmo_sccp_user *scu, struct osmo_prim_hdr *op
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 */
+/*! Main entrance function for primitives from SCCP User.
+ * Implies a msgb_free(oph->msg), otherwise the same as sccp_sclc_user_sap_down_nofree().
+ * \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_down_nofree(scu, oph);
msgb_free(msg);
-
return rc;
}
diff --git a/src/sccp_scoc.c b/src/sccp_scoc.c
index 91a1ab7..7570764 100644
--- a/src/sccp_scoc.c
+++ b/src/sccp_scoc.c
@@ -1694,15 +1694,15 @@ static uint32_t scu_prim_conn_id(const struct osmo_scu_prim *prim)
}
}
-/*! \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, otherwise the same as osmo_sccp_user_sap_down().
* \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_down_nofree(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;
@@ -1714,7 +1714,7 @@ int osmo_sccp_user_sap_down(struct osmo_sccp_user *scu, struct osmo_prim_hdr *op
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_down_nofree(scu, oph);
case OSMO_PRIM(OSMO_SCU_PRIM_N_CONNECT, PRIM_OP_REQUEST):
/* Allocate new connection structure */
conn = conn_create_id(scu, prim->u.connect.conn_id);
@@ -1722,7 +1722,7 @@ int osmo_sccp_user_sap_down(struct osmo_sccp_user *scu, struct osmo_prim_hdr *op
/* 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;
}
break;
case OSMO_PRIM(OSMO_SCU_PRIM_N_CONNECT, PRIM_OP_RESPONSE):
@@ -1735,25 +1735,33 @@ int osmo_sccp_user_sap_down(struct osmo_sccp_user *scu, struct osmo_prim_hdr *op
/* 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);
+}
+/*! Main entrance function for primitives from SCCP User.
+ * Implies a msgb_free(oph->msg), otherwise the same as osmo_sccp_user_sap().
+ * \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_down_nofree(scu, oph);
+ msgb_free(msg);
return rc;
}