Skip to content

tests/fuzz/fuzz-open_channel: fix bad local variable access after longjmp #9165

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.

Sign up for GitHub

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

Merged
daywalker90 merged 1 commit into from
Aug 17, 2026
+3 −3
Merged

tests/fuzz/fuzz-open_channel: fix bad local variable access after longjmp #9165

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions tests/fuzz/fuzz-open_channel.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -439,14 +439,14 @@ void init(int *argc, char ***argv)

void run(const u8 *data, size_t size)
{
if (setjmp(fuzz_env) != 0)
goto cleanup;

/* The function under test: fundee_channel(), calls
* clean_tmpctx() mid-run, so create a separate context.
*/
const tal_t *run_ctx = tal(NULL, tal_t);

if (setjmp(fuzz_env) != 0)

@Andezion Andezion Aug 4, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think, did tests/fuzz/fuzz-handle_onion_message.c (also using jmp_buf fuzz_env and setjmp(fuzz_env) at line 78) has the same ordering issue?

@whitslack whitslack Aug 11, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of setjmp/longjmp in tests/fuzz/fuzz-handle_onion_message.c shouldn't cause a crash since the only local variable (daemon) accessed after the longjmp is initialized (to NULL) before the setjmp. However, the call to tal_free(daemon->master) may or may not happen as intended, depending on whether the compiler emits instructions to reload the register holding the value of daemon after the longjmp. If the register is reloaded (from the stack), then it may hold the non-null address to which daemon was set from the return value of new_daemon() (depending on whether the compiler emitted instructions after the call to new_daemon() to flush the new value of daemon back onto the stack), and tal_free(daemon->master) will be called if this has occurred. On the other hand, if the register is not reloaded, then it will still hold the value NULL (the value to which daemon was initialized before the setjmp call), and tal_free(daemon->master) will not be called. I would argue that it's not a good idea to have control flow vary depending upon compiler optimizations. You can prevent the compiler from caching the daemon local variable in a register by declaring it (i.e., the pointer itself, not the pointed-to object) volatile, but I generally wouldn't recommend that, as volatile is detrimental to compiler optimizations. A nicer fix would be to insert a second call to setjmp after daemon is set to the return value from new_daemon(). That would ensure that the code at the cleanup label will always see the latest value of daemon, even in the case that cleanup is reached via a longjmp.

goto cleanup;

/* Initialize the global pointers to the fuzz data. */
cursor = &data;
max = &size;
Expand Down
Loading