Neels Hofmeyr has submitted this change and it was merged. (
https://gerrit.osmocom.org/13277 )
Change subject: add caller-owns-msgb variant osmo_sccp_user_sap_down_nofree()
......................................................................
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
---
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, 38 insertions(+), 23 deletions(-)
Approvals:
Jenkins Builder: Verified
osmith: Looks good to me, but someone else must approve
Harald Welte: Looks good to me, approved
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_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 @@
/* 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 @@
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 @@
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 @@
}
}
-/*! \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 @@
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 @@
/* 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 @@
/* 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;
}
--
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: merged
Gerrit-Change-Id: Ic818efa78b90f727e1a94c18b60d9a306644f340
Gerrit-Change-Number: 13277
Gerrit-PatchSet: 2
Gerrit-Owner: Neels Hofmeyr <nhofm...@sysmocom.de>
Gerrit-Reviewer: Harald Welte <lafo...@gnumonks.org>
Gerrit-Reviewer: Jenkins Builder (1000002)
Gerrit-Reviewer: Neels Hofmeyr <nhofm...@sysmocom.de>
Gerrit-Reviewer: osmith <osm...@sysmocom.de>
