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

worker: Do not use references for async callbacks #1274

Merged
merged 2 commits into from Dec 20, 2023

Conversation

jmillan
Copy link
Member

@jmillan jmillan commented Dec 20, 2023

If the callback is not executed in the same uv_loop iteration, the access to such values upon executing the callback is undefined.

If the callback is not executed in the same uv_loop iteration, the
access to such values upon executing the callback is undefined.
@jmillan jmillan requested a review from ibc December 20, 2023 14:34
@jmillan
Copy link
Member Author

jmillan commented Dec 20, 2023

TODO: CHANGELOG

@ibc
Copy link
Member

ibc commented Dec 20, 2023

What is this fixing?

@jmillan
Copy link
Member Author

jmillan commented Dec 20, 2023

What is this fixing?

If the packet is not sent in the current loop iteration, ie: if uv_udp_try_send() cannot send the packet immediately and we need to use uv_udp_send() which will send the packet when possible (not in this iteration) and notify us when that is done, we will run the send callback in the future. In such future, the variables passed by reference to the callback will NOT exist, because they were in the stack, back in such previous iteration when uv_udp_send() was called.

Accessing references to stack variables that do not exist anymore has undefined behaviour, so we may be probably calling the transport congestion PacketSent() methods with garbage.

Also, this reminded me that we were having a crash with jemalloc at work, and this may well be related.

@ibc
Copy link
Member

ibc commented Dec 20, 2023

I see. Good. Approved, like my txuleton. Look:

image

@jmillan
Copy link
Member Author

jmillan commented Dec 20, 2023

I see. Good. Approved, like my txuleton

0xdeadbeef

@ibc
Copy link
Member

ibc commented Dec 20, 2023

Plus good dessert and 3 wines and patxaran

@jmillan jmillan merged commit 8365c62 into v3 Dec 20, 2023
36 checks passed
@jmillan jmillan deleted the dont-use-references-for-async-callbacks branch December 20, 2023 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants