summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorNeels Hofmeyr <neels@hofmeyr.de>2019-03-10 04:41:27 +0100
committerNeels Hofmeyr <neels@hofmeyr.de>2019-03-10 04:41:28 +0100
commit9d892e17f9a4032176e72e8373d6df83cb859bca (patch)
treee12488d6e6a0249db527a6899997b3b0216c8aae
parent8781edf93dc7307ecfe668fb3f9f9e1d557e463b (diff)
add caller-owns-msgb variant osmo_sccp_user_sap_down2()neels/caller_owns_msgb
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
-rw-r--r--include/osmocom/sigtran/sccp_sap.h1
-rw-r--r--src/sccp_internal.h1
-rw-r--r--src/sccp_sclc.c26
-rw-r--r--src/sccp_scoc.c31
4 files changed, 36 insertions, 23 deletions
diff --git a/include/osmocom/sigtran/sccp_sap.h b/include/osmocom/sigtran/sccp_sap.h
index f8cb686..bf2bc70 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_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 @@ 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_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 @@ 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.
* \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 @@ 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 */
+/*! 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 @@ 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.
* \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 @@ 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_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 @@ 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;
}
conn->user = scu;
break;
@@ -1729,25 +1729,32 @@ 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);
+}
+/*! 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;
}