Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

jv_setpath: fix leak when indexing an array with an array #3083

Merged
merged 1 commit into from
Mar 28, 2024

Conversation

emanuele6
Copy link
Member

arrays[arrays] is a special case of "INDEX" that actually returns an
array containing the indices in which the array that is being indexed
contains the start of the key array.

So array keys, for array values, are a kind of key that can be "got",
but not "set". jv_setpath() was not freeing the value it "got" from
indexing that key, in case the following "set" on that key failed,
resulting in a leak.

$ ./jq -n '[] | setpath([[1]]; 1)'
jq: error (at <unknown>): Cannot update field at array index of array

=================================================================
==953483==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 272 byte(s) in 1 object(s) allocated from:
    #0 0x725f4d4e1359 in __interceptor_malloc /usr/src/debug/gcc/gcc/libsanitizer/asan/asan_malloc_linux.cpp:69
    #1 0x5ec17b1a7438 in jv_mem_alloc src/jv_alloc.c:141

SUMMARY: AddressSanitizer: 272 byte(s) leaked in 1 allocation(s).

Fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=66061


This one was hard to figure out because oss-fuzz really didn't do a
great job minimising the reproducer, and the stacktrace was not very
helpful.
After figuring out the leak was in jv_setpath(), and it had something
to do with using an array key, I had to keep track of copies/frees in
jv_setpath() on a piece of paper while stepping the code with gdb to
find the leak! :^)

arrays[arrays] is a special case of "INDEX" that actually returns an
array containing the indices in which the array that is being indexed
contains the start of the key array.

So array keys, for array values, are a kind of key that can be "got",
but not "set". jv_setpath() was not freeing the value it "got" from
indexing that key, in case the following "set" on that key failed,
resulting in a leak.

    $ ./jq -n '[] | setpath([[1]]; 1)'
    jq: error (at <unknown>): Cannot update field at array index of array

    =================================================================
    ==953483==ERROR: LeakSanitizer: detected memory leaks

    Direct leak of 272 byte(s) in 1 object(s) allocated from:
        #0 0x725f4d4e1359 in __interceptor_malloc /usr/src/debug/gcc/gcc/libsanitizer/asan/asan_malloc_linux.cpp:69
        jqlang#1 0x5ec17b1a7438 in jv_mem_alloc src/jv_alloc.c:141

    SUMMARY: AddressSanitizer: 272 byte(s) leaked in 1 allocation(s).

Fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=66061
@emanuele6
Copy link
Member Author

@emanuele6 emanuele6 added the bug label Mar 28, 2024
@emanuele6
Copy link
Member Author

hmm, installing firefox?!?!
file

@wader
Copy link
Member

wader commented Mar 28, 2024

hmm, installing firefox?!?! file

Seems so :) https://github.com/actions/runner-images/blob/main/images/ubuntu/Ubuntu2204-Readme.md#browsers-and-drivers

…also sadly old jq 1.6 :(

@emanuele6 emanuele6 merged commit 5bbd02f into jqlang:master Mar 28, 2024
28 checks passed
@emanuele6 emanuele6 deleted the foobar branch March 28, 2024 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants