Fix use-after-free in filter() / map() plus some other stuff#407
Open
MattCatz wants to merge 7 commits into
Open
Fix use-after-free in filter() / map() plus some other stuff#407MattCatz wants to merge 7 commits into
MattCatz wants to merge 7 commits into
Conversation
Author
|
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. |
Contributor
|
I think the fixup should be folded into the commit. Also the json fix would probably be a bit cleaner with a |
`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>
Author
|
Let me know if you see anything else @nbd168 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.