From ac729eb5a8295d0c1fc2dea3fb0f56413654c13e Mon Sep 17 00:00:00 2001 From: Neels Hofmeyr Date: Sun, 10 Mar 2019 04:41:27 +0100 Subject: 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 --- include/osmocom/sigtran/sccp_sap.h | 1 + src/sccp_internal.h | 1 + src/sccp_sclc.c | 27 ++++++++++++++++----------- src/sccp_scoc.c | 32 ++++++++++++++++++++------------ 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; } -- cgit v1.2.3