Missing NULL checks - qmi-proxy.c

[Aleksander Morgado]

Hey Peter

> Here are some trivial null checks I found during testing.  I believe there are
> some more issues remaining.  This is against both latest release 1.2.26.6 and
> current git.
>

Are any of these new checks added to fix bugs? Or just through code review?
See comments below.

> --- a/src/libqmi-glib/qmi-proxy.c       2020-12-17 20:07:45.620241110 -0500
> +++ b/src/libqmi-glib/qmi-proxy.c       2020-12-17 20:21:32.405975476 -0500
> @@ -212,7 +212,7 @@
>   {
>       guint i;
>
> -    if (!client->qmi_client_info_array->len)
> +    if (!client->qmi_client_info_array || !client->qmi_client_info_array->len)
>           return;

The client->qmi_client_info_array is allocated as soon as the Client
struct is allocated, and the array is removed when the Client struct
is freed; so there is (should be) no chance that this array is every
NULL.

Now I'm really interested on how you got to the point to need to fix
this, because there has been one other user also providing the same
kind of fix, see:
https://gitlab.freedesktop.org/mobile-broadband/libqmi/-/merge_requests/181

In that MR above, the backtrace and sequence of events leading to this
problem indicated that the problem was NOT really the lack of checking
if array was NULL before de-referencing it. Instead, the problem could
have been that the Client struct was already freed (and so the array
re-set to NULL) and then the crash happened. The solution is not to
check if array is NULL or not, the solution should be to avoid the
use-after-free.

If you have a way to reproduce this issue, please run the qmi-proxy
under valgrind:
https://gitlab.freedesktop.org/mobile-broadband/libqmi/-/merge_requests/181#note_716877

We should see "invalid write" errors if that theory makes sense.

>
>       for (i = 0; i < client->qmi_client_info_array->len; i++) {
> @@ -311,6 +311,8 @@
>   {
>       guint i;
>
> +    if (!client->qmi_client_info_array) return;
> +

Same as above. Also, we would put the return in the next line ;)

>       for (i = 0; i < client->qmi_client_info_array->len; i++) {
>           QmiClientInfo *info;
>
> @@ -541,6 +543,8 @@
>   {
>       guint i;
>
> +    if (!array) return -1;
> +

Same as above, as qmi_client_info_array_lookup_cid() takes as input
the client->qmi_client_info_array.

>       for (i = 0; i < array->len; i++) {
>           QmiClientInfo *item;
>
> _______________________________________________
> libqmi-devel mailing list
> libqmi-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/libqmi-devel



-- 
Aleksander
https://aleksander.es