On Thu, 2017-08-03 at 13:25 -0700, Ben Chan wrote: > This patch fixes some potential use-after-freed issues in > dms_get_ids_ready(). When an invalid ESN / MEID is retrieved, > `ctx->self->priv->esn' / `ctx->self->priv->meid' is freed but not > reset > to NULL. If no IMEI is retrieved, `str' can be set to the already > freed > `ctx->self->priv->esn' / `ctx->self->priv->meid' and then propagated > to > a GSimpleAsyncResult object. > --- > src/mm-broadband-modem-qmi.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/src/mm-broadband-modem-qmi.c b/src/mm-broadband-modem- > qmi.c > index 38356426..bd4d2b6c 100644 > --- a/src/mm-broadband-modem-qmi.c > +++ b/src/mm-broadband-modem-qmi.c > @@ -1237,8 +1237,10 @@ dms_get_ids_ready (QmiClientDms *client, > ctx->self->priv->esn = g_strdup_printf ("0%s", str); /* > zero-pad to 8 chars */ > else if (len == 8) > ctx->self->priv->esn = g_strdup (str); > - else > + else { > mm_dbg ("Invalid ESN reported: '%s' (unexpected > length)", str); > + ctx->self->priv->esn = NULL;
i'm tempted to say that just above here we should just do: g_clear_pointer (&ctx->self->priv->esn, g_free); and the same for MEID. Then we don't forget some time later if we add more error paths or something. I'm a big fan of free-and-clear in one place. We could also make a macro/inline for this if we want, like "mm_clear_pointer()" that just does the above. Dan > + } > } > > if (qmi_message_dms_get_ids_output_get_meid (output, &str, NULL) > && > @@ -1247,8 +1249,10 @@ dms_get_ids_ready (QmiClientDms *client, > len = strlen (str); > if (len == 14) > ctx->self->priv->meid = g_strdup (str); > - else > + else { > mm_dbg ("Invalid MEID reported: '%s' (unexpected > length)", str); > + ctx->self->priv->meid = NULL; > + } > } > > if (ctx->self->priv->imei) _______________________________________________ ModemManager-devel mailing list ModemManager-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel