Various leak/cycle fixes#410
Merged
Merged
Conversation
ucv_prototype_set() overwrote the prototype pointer without releasing the previous prototype, even though ucv_free() does drop it. Anything that re-installs a prototype, notably the ubus binding installing a fresh request prototype on every incoming call, leaked the old one each time. Release the previous prototype before storing the new one, matching the ownership convention of ucv_object_add() and ucv_array_push(). Signed-off-by: John Crispin <john@phrozen.org>
uc_uloop_task_clear() closed the task output pipe via uloop_fd_close() without first calling uloop_fd_delete(). The descriptor stayed registered with the event loop and any pending cur_fds[] entries were left dangling, causing a use-after-free once the task struct was freed. Signed-off-by: Felix Fietkau <nbd@nbd.name>
Callback resources can form a reference cycle: the resource owns a closure (the callback) whose upvalues capture the resource itself, either directly or via a wrapping userdata object. Pure reference counting cannot break such a cycle on its own; the resource and its captured closure would only ever be reclaimed by a subsequent mark-and-sweep gc pass, and would otherwise be pinned for the lifetime of the process. When releasing the callback resource in uc_uloop_cb_free(), drop all of its stored upvalues first. This severs the cycle deterministically at the point where uloop is done with the resource, allowing the closure and any data it captured to be freed immediately via refcount, without waiting for gc to come around. Signed-off-by: Felix Fietkau <nbd@nbd.name>
ubus connection and object resources can form a reference cycle: the resource owns closures (disconnect/notify/request callbacks) whose upvalues capture the resource itself, either directly or via a wrapping userdata object. Pure reference counting cannot break such a cycle on its own; the resource and its captured closures would only ever be reclaimed by a subsequent mark-and-sweep gc pass, and would otherwise be pinned for the lifetime of the process. In the connection case this also leaked the underlying socket fd on every reconnect. When releasing the resource in uc_ubus_put_res(), drop all of its stored upvalues first. This severs the cycle deterministically at the point where ubus is done with the resource, allowing the closures and any data they captured to be freed immediately via refcount, without waiting for gc to come around. Reported-by: John Crispin <john@phrozen.org> Signed-off-by: Felix Fietkau <nbd@nbd.name>
…onnect uc_ubus_channel_disconnect_cb invokes the user-supplied disconnect_cb before tearing down the connection. The uc_ubus_connection_t (c) lives in the payload of c->res, so its lifetime is tied to the resource. If the handler calls c.disconnect() on the same channel, uc_ubus_disconnect runs ubus_shutdown and drops the last reference via uc_ubus_put_res, freeing c. After the callback returns, the outer function still dereferences c->buf, c->ctx, c->fd_handle and c->res. Pin an extra reference to c->res across the user callback and the subsequent teardown so the resource cannot be freed until the outer function is done with it. Signed-off-by: Felix Fietkau <nbd@nbd.name>
Owner
|
Merged, thanks! |
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
3 participants
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.
Break potential reference cycles by clearing resource held values.
Fix fd leak on uloop tasks
Fix potential use-after-free on re-entrant ubus channel disconnect calls
Fix refcount leak on setting prototypes