sgsn: Prevent memory leak and double free

This has been re-produced using the "osmo-pcu emulator" code
and a ping to force segmented SNDCP messages. When the NS link
enters the DEAD/BLOCKED state the msgb would be freed twice.
Once inside gprs_ns_sendmsg and once by the caller. Based on the
return one can not see if the parameter has been deleted.

I changed libosmocore/libosmogb to always free the msgb in case
of an error on the way to gprs_ns_sendmsg. Catch up, avoid the
double free and fix some memory leaks. In case the sending fails
assume the entire segmented message is at end and free the
original input data.

This has been tested by posix suspending/resuming the emulator
process to have the GPRS-NS link go to dead/blocked to alive
and unblocked. The ping recovers and "SIGUSR1" to the SGSN does
not show active memory allocations.

The SGSN calls bssgp_tx_dl_ud at the lowest level and has the
following callchains. Most of them allocate the msgb and have
no early return and transfer ownership already:

<- gprs_llc_tx_u
<- gprs_llc_tx_ui
	<- gsm48_gmm_sendmsg (all callers sane)
		<- _tx_status
		<- _tx_detach_req
	<- gprs_llc_tx_xid (all callers sane)
	<- sndcp_unitdata_req
		<- sndcp_send_ud_frag

2 files changed, 8 insertions, 3 deletions

diff --git a/openbsc/src/gprs/gprs_llc.c b/openbsc/src/gprs/gprs_llc.c
index cb810c4a2..c88d16269 100644
--- a/openbsc/src/gprs/gprs_llc.c
+++ b/openbsc/src/gprs/gprs_llc.c
@@ -382,6 +382,7 @@ int gprs_llc_tx_ui(struct msgb *msg, uint8_t sapi, int command,
 	if (msg->len > lle->params.n201_u) {
 		LOGP(DLLC, LOGL_ERROR, "Cannot Tx %u bytes (N201-U=%u)\n",
 			msg->len, lle->params.n201_u);
+		msgb_free(msg);
 		return -EFBIG;
 	}
 
@@ -439,6 +440,7 @@ int gprs_llc_tx_ui(struct msgb *msg, uint8_t sapi, int command,
 				     kc, iv, GPRS_CIPH_SGSN2MS);
 		if (rc < 0) {
 			LOGP(DLLC, LOGL_ERROR, "Error crypting UI frame: %d\n", rc);
+			msgb_free(msg);
 			return rc;
 		}
 
diff --git a/openbsc/src/gprs/gprs_sndcp.c b/openbsc/src/gprs/gprs_sndcp.c
index 98da4aea8..46d779b4c 100644
--- a/openbsc/src/gprs/gprs_sndcp.c
+++ b/openbsc/src/gprs/gprs_sndcp.c
@@ -358,8 +358,10 @@ static int sndcp_send_ud_frag(struct sndcp_frag_state *fs)
 
 	fmsg = msgb_alloc_headroom(fs->sne->lle->params.n201_u+256, 128,
 				   "SNDCP Frag");
-	if (!fmsg)
+	if (!fmsg) {
+		msgb_free(fs->msg);
 		return -ENOMEM;
+	}
 
 	/* make sure lower layers route the fragment like the original */
 	msgb_tlli(fmsg) = msgb_tlli(fs->msg);
@@ -416,9 +418,9 @@ static int sndcp_send_ud_frag(struct sndcp_frag_state *fs)
 	sch->more = more;
 
 	rc = gprs_llc_tx_ui(fmsg, lle->sapi, 0, fs->mmcontext);
+	/* abort in case of error, do not advance frag_nr / next_byte */
 	if (rc < 0) {
-		/* abort in case of error, do not advance frag_nr / next_byte */
-		msgb_free(fmsg);
+		msgb_free(fs->msg);
 		return rc;
 	}
 
@@ -450,6 +452,7 @@ int sndcp_unitdata_req(struct msgb *msg, struct gprs_llc_lle *lle, uint8_t nsapi
 	sne = gprs_sndcp_entity_by_lle(lle, nsapi);
 	if (!sne) {
 		LOGP(DSNDCP, LOGL_ERROR, "Cannot find SNDCP Entity\n");
+		msgb_free(msg);
 		return -EIO;
 	}