Openconnect crashes on auth dialog

I did some debugging and found the reason for the crash: an array is attempted to be freed even though it could be NULL.

Below is the code fragment with the missing if-statenment inserted.

In auth-dialog/main.c around line 720:

function static void cookie_cb (GObject *source_obj, GAsyncResult *res, gpointer data):
[...]
    for (i = 0; i < 2 * (num_cookies + 1); i++) {
        free(cookie_array[i]);
    }
    free(cookie_array);

    if (headers_array) { // <=========== THIS LINE IS MISSING AND MUST BE ADDED
        for (i = 0; i < 2 * (num_headers + 1); i++) {
            free(headers_array[i]);
        }
        free(headers_array);
    } // <=========== THIS LINE IS MISSING AND MUST BE ADDED
    if (headers)
        soup_message_headers_free (headers);

The attachment "Patch to avoid freeing pointer if NULL." seems to be a patch. If it isn't, please remove the "patch" flag from the attachment, remove the "patch" tag, and if you are a member of the ~ubuntu-reviewers, unsubscribe the team.

[This is an automated message performed by a Launchpad user owned by ~brian-murray, for any issues please contact him.]

Status changed to 'Confirmed' because the bug affects multiple users.

Proposed patch fixes the issue for me.

Hm, that's odd. It should only actually be freeing anything there if the `num_headers` count is greater than zero. In which case it *should* have allocated `headers_array`. Unless the calloc() failed, but that is unlikely.

Then again, I'm looking at the upstream code. And that has had the check for `headers_array` being NULL since it was first committed in https://gitlab.gnome.org/GNOME/NetworkManager-openconnect/-/commit/9089be31#5122bcd6270f3f91d7aa6174306705bad477d30f_699_721

I'd suggest reconciling with the upstream version of the code.

It looks to me like the bug is still in the upstream code. `headers_array` is only allocated if `headers` is not null, and that will be null if `resource` or `response` is null.

Yes, but num_headers never changes from zero in that case, so no attempt is made to free anything.

And also, the loop which does the freeing has the check for headers_array being NULL anyway, as part of the termination condition:

 for (i = 0; headers_array && i < 2 * (num_headers + 1); i++) {
  free(headers_array[i]);
 }

I still don't quite see *how* it can happen when num_headers is non-zero though, unless the calloc() fails. Or unless openconnect_webview_load_changed() frees them, which surely it shouldn't?

This check for headers_array in the loop’s termination condition is only in the upstream code, not in the one that was in Ubuntu at the time I reported the bug and submitted the patch.

When I made the patch I decided to put a proper if-clause there because it’s easier to understand what is going on (the check in the for loop seems like obfuscated programming to me) and technically it’s even faster to check only once instead of at every loop iteration (even though this minimal performance gain is hardly significant here).

> This check for headers_array in the loop’s termination condition is only in the upstream code, not in the one that was in Ubuntu at the time I reported the bug and submitted the patch.

Yes. That's why I'm pointing out the difference and suggesting that we reconcile the code.

> When I made the patch I decided to put a proper if-clause there because it’s easier to understand what is going on (the check in the for loop seems like obfuscated programming to me)

Agreed. Happy to accept a patch which cleans that up in the upstream tree. Although...

> and technically it’s even faster to check only once instead of at every loop iteration (even though this minimal performance gain is hardly significant here).

In practice any compiler worth its salt should optimise that away because it'll know that calling free() won't affect the value of headers_array. See in 'objdump -S' output how the loop only jump back from 0x8d98 to 0x9d88, adding 8 to the pointer every time until it reaches the precalculated end value.

        free(cookie_array);
    8d6a: 48 8b 3c 24 mov (%rsp),%rdi
    8d6e: e8 ed da ff ff call 6860 <free@plt>
        for (i = 0; headers_array && i < 2 * (num_headers + 1); i++) {
    8d73: 4d 85 ff test %r15,%r15
    8d76: 74 22 je 8d9a <cookie_cb+0x25a>
    8d78: 43 8d 44 24 01 lea 0x1(%r12,%r12,1),%eax
    8d7d: 4c 89 fd mov %r15,%rbp
    8d80: 4d 8d 64 c7 08 lea 0x8(%r15,%rax,8),%r12
    8d85: 0f 1f 00 nopl (%rax)
                free(headers_array[i]);
    8d88: 48 8b 7d 00 mov 0x0(%rbp),%rdi
        for (i = 0; headers_array && i < 2 * (num_headers + 1); i++) {
    8d8c: 48 83 c5 08 add $0x8,%rbp
                free(headers_array[i]);
    8d90: e8 cb da ff ff call 6860 <free@plt>
        for (i = 0; headers_array && i < 2 * (num_headers + 1); i++) {
    8d95: 49 39 ec cmp %rbp,%r12
    8d98: 75 ee jne 8d88 <cookie_cb+0x248>
        free(headers_array);
    8d9a: 4c 89 ff mov %r15,%rdi
    8d9d: e8 be da ff ff call 6860 <free@plt>