Fix use-after-free in filter() / map() plus some other stuff#407
Fix use-after-free in filter() / map() plus some other stuff#407MattCatz wants to merge 8 commits into
Conversation
|
I believe the CI check is a false alarm. It seems to be fixed in newer versions of gcc (CI uses gcc10?). I'll make a push to keep CI happy. |
|
I think the fixup should be folded into the commit. Also the json fix would probably be a bit cleaner with a |
|
Let me know if you see anything else @nbd168 |
`ucv_array_new` returns an array with a ref count of 1. `ucv_get(cbs)` adds an extra reference and the array is never free-ed. Signed-off-by: Matthew Cather <mattbob4@gmail.com>
53 - filter/map/callfunc: result/scope freed during a callback gc("collect")
54 - json_from_object: cached read method dangles after obj.read reassignment
55 - zlib def_object/inf_object
56 - socket.poll: result array freed during a user fileno() callback
57 - uvcount=0 resources leaked when reached via a retain=true sweep
Signed-off-by: Matthew Cather <mattbob4@gmail.com>
uc_json_from_object cached the read method via ucv_property_get, which returns a borrow. If the user's read callback reassigned obj.read, the original closure's last owning reference vanished when the call frame popped, leaving the cached pointer dangling for the next iteration's ucv_get. Signed-off-by: Matthew Cather <mattbob4@gmail.com>
uc_filter and uc_map hold their result array in a C local; uc_callfunc
holds the saved previous globals scope. A gc("collect") triggered from
inside the user callback freed those values and left the builtin to
dereference a dangling pointer once control returned.
Push each onto the VM stack with ucv_get so they're rooted via vm->stack
across the call.
Signed-off-by: Matthew Cather <mattbob4@gmail.com>
A gc("collect") from inside a polled handle's fileno() method freed
the C-local result array out from under uc_socket_poll. Push it onto
the VM stack so it stays rooted across the call.
uc_fn_arg(i) peeks from the top, so it now sees the anchor instead of
the args; switch the loop to direct stack-index access via argoff.
Signed-off-by: Matthew Cather <mattbob4@gmail.com>
uc_zlib_def_object and uc_zlib_inf_object cached the read method via ucv_property_get, which returns a borrow. If the user's read callback reassigned obj.read, the original closure's last owning reference vanished when the callframe popped, leaving the cached pointer dangling for the next iteration's ucv_get. Take an owning reference and release it at every exit path. Signed-off-by: Matthew Cather <mattbob4@gmail.com>
ucv_free's retain=true path defers free() and expects the GC's second pass to mop up UC_NULL items from vm->values. But resources with uvcount=0 aren't on that list — ucv_resource_new_ex skips them because leaf resources can't form cycles. So a retain=true sweep cascading into one set type=UC_NULL and walked away; the memory leaked forever. Only defer when ref is actually linked into a list. Signed-off-by: Matthew Cather <mattbob4@gmail.com>
|
The failure is a pre-existing bug in When we mess with the print_buf (e.x. It only appears against json-c v15 and older. Newer versions of json-c hide the issue since they memset the 'gap' to null, which leaves a null terminator where ours should be. json-c/json-c@63c602f |
The reserve only wrote one byte at the end of the CHUNK window, so after
inflate()/deflate() advanced bpos, buf[bpos] was left uninitialised and the
printbuf was no longer NUL terminated. printf("%s") then ran off the end of
the data into garbage. json-c 0.16+ masked it by zeroing the gap itself;
0.15 didn't.
That one-byte trick avoided zeroing all of CHUNK, but it's pointless now:
0.16+ zero-fills the gap anyway. Just memset the whole window.
Signed-off-by: Matthew Cather <mattbob4@gmail.com>
|
Was the closure intentional? I do have this PR on my radar and plan to push it, but I did not get around to investigate the chunk overrun myself yet. Edit: nevermind, I see you reopened already |
|
Closing was just a brain fart :D. As far as I can tell, I changed it to just clear the whole buffer. If you don't like that then |
3 participants
This should address: #406
While working on the original issue I came across a few other gc related things and have also included them.
I tried to break everything down into its own commit in case you want to drop one of the fixes for something better. I added a test case for each of the issues. You should be able to recreate the issues locally by running the test cases with ASAN or valgrind (I used ASAN locally).
Open to feedback.