summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorNeels Hofmeyr <neels@hofmeyr.de>2019-04-02 04:23:50 +0200
committerNeels Hofmeyr <neels@hofmeyr.de>2019-04-02 05:17:30 +0200
commitb34ac5621973da3d5cbd5a36e75767d3fb7788b5 (patch)
treed1d451ebd579f692a4a44c1626fb7367ccac951a
parent7ca3ecef2f929e33c72fbcc66c65b390ce4349a0 (diff)
fix read_cb_forward(): avoid segfault when error logging
msg is already deallocated, must use the copied gsup_err-> data instead. Change-Id: I9fd623b6654fd849fb0aabc288da64c238a9050b
-rw-r--r--src/hlr.c23
1 files changed, 15 insertions, 8 deletions
diff --git a/src/hlr.c b/src/hlr.c
index bcd1c01..a079c19 100644
--- a/src/hlr.c
+++ b/src/hlr.c
@@ -447,6 +447,9 @@ static int read_cb_forward(struct osmo_gsup_conn *conn, struct msgb *msg, const
int ret = -EINVAL;
struct osmo_gsup_message *gsup_err;
+ /* FIXME: it would be better if the msgb never were deallocated immediately by osmo_gsup_addr_send(), which a
+ * select-loop volatile talloc context could facilitate. Then we would still be able to access gsup-> members
+ * (pointing into the msgb) even after sending failed, and we wouldn't need to copy this data before sending: */
/* Prepare error message (before IEs get deallocated) */
gsup_err = talloc_zero(hlr_ctx, struct osmo_gsup_message);
OSMO_STRLCPY_ARRAY(gsup_err->imsi, gsup->imsi);
@@ -476,17 +479,20 @@ static int read_cb_forward(struct osmo_gsup_conn *conn, struct msgb *msg, const
osmo_quote_str((const char *)gsup->destination_name, gsup->destination_name_len));
msg->data = msgb_l2(msg); /* Remove old IPA header */
ret = osmo_gsup_addr_send(g_hlr->gs, gsup->destination_name, gsup->destination_name_len, msg);
+ /* AT THIS POINT, THE msg MAY BE DEALLOCATED and the data like gsup->imsi, gsup->source_name etc may all be
+ * invalid and cause segfaults. */
msg = NULL;
+ gsup = NULL;
if (ret == -ENODEV)
- LOGP(DMAIN, LOGL_ERROR, "Can't forward GSUP message for IMSI %s from %s to %s: destination not"
- " connected\n", gsup->imsi,
- osmo_quote_str_buf((const char *)gsup->source_name, gsup->source_name_len, buf, sizeof(buf)),
- osmo_quote_str((const char *)gsup->destination_name, gsup->destination_name_len));
+ LOGP(DMAIN, LOGL_ERROR,
+ "Can't forward GSUP message for IMSI %s from %s to %s: destination not connected\n", gsup_err->imsi,
+ osmo_quote_str_buf((const char *)gsup_err->source_name, gsup_err->source_name_len, buf, sizeof(buf)),
+ osmo_quote_str((const char *)gsup_err->destination_name, gsup_err->destination_name_len));
else if (ret)
LOGP(DMAIN, LOGL_ERROR, "Can't forward GSUP message for IMSI %s from %s to %s: unknown error\n",
- gsup->imsi,
- osmo_quote_str_buf((const char *)gsup->source_name, gsup->source_name_len, buf, sizeof(buf)),
- osmo_quote_str((const char *)gsup->destination_name, gsup->destination_name_len));
+ gsup_err->imsi,
+ osmo_quote_str_buf((const char *)gsup_err->source_name, gsup_err->source_name_len, buf, sizeof(buf)),
+ osmo_quote_str((const char *)gsup_err->destination_name, gsup_err->destination_name_len));
end:
/* Send error back to source */
@@ -498,7 +504,8 @@ end:
osmo_gsup_conn_send(conn, msg_err);
}
talloc_free(gsup_err);
- msgb_free(msg);
+ if (msg)
+ msgb_free(msg);
return ret;
}