fuzz-tests: Add a test for `peer_init_received()` by Chand-ra · Pull Request #8385 · ElementsProject/lightning

Chand-ra · 2025-06-30T04:07:07Z

peer_init_received() in connectd/peer_exchange_initmsg.{c, h} is responsible for handling init messages defined in BOLT #1. Since it deals with untrusted input, add a test for it.

Checklist

Before submitting the PR, ensure the following tasks are completed. If an item is not applicable to your PR, please mark it as checked:

The changelog has been updated in the relevant commit(s) according to the guidelines.
Tests have been added or modified to reflect the changes.
Documentation has been reviewed and updated as needed.
Related issues have been listed and linked, including any that this PR closes.

CC: @morehouse

Chand-ra · 2025-06-30T04:10:41Z

This test also triggers the bug that is fixed in #8325.

morehouse

This fuzz target leaks memory because peer_init_received uses tmpctx and clean_tmpctx is never called.

Also we should add better comments, given the complexity of this target.

morehouse · 2025-06-30T14:52:22Z

tests/fuzz/fuzz-init_received.c

@@ -0,0 +1,161 @@
+#include "config.h"


Please add a header comment here explaining how this fuzz target works.

morehouse · 2025-06-30T14:53:34Z

tests/fuzz/fuzz-init_received.c

+static size_t io_buf_pos = 0;
+
+static struct io_plan *
+test_write(struct io_conn *conn, const void *data, size_t len,


Intercepting io_read and io_write is tricky, so please document exactly why we do this and what we are trying to accomplish here.

Still missing documentation on what exactly test_read and test_write are doing, and why. If I can't figure out what's going on here, probably no one else can either.

morehouse · 2025-06-30T14:55:43Z

tests/fuzz/fuzz-init_received.c

+	conn = io_new_conn(run_ctx, -1, do_nothing, NULL);
+	peer_init_received(conn, peer);
+
+	tal_free(run_ctx);


We should probably just use tmpctx for simplicity.

Right, that's what I was doing initially but calling clean_tmpctx() within the target causes the following crash:

fuzz-init_received: ccan/ccan/tal/tal.c:428: void del_tree(struct tal_hdr *, const tal_t *, int): Assertion `!taken(from_tal_hdr(t))' failed. ==10716== ERROR: libFuzzer: deadly signal #0 0x599305aa8e05 in __sanitizer_print_stack_trace (/home/chandra/lightning/tests/fuzz/fuzz-init_received+0x292e05) (BuildId: 2e1951a325c952a9be03b7b50fae5fe68183b738) #1 0x599305a0291c in fuzzer::PrintStackTrace() (/home/chandra/lightning/tests/fuzz/fuzz-init_received+0x1ec91c) (BuildId: 2e1951a325c952a9be03b7b50fae5fe68183b738) #2 0x5993059e89a7 in fuzzer::Fuzzer::CrashCallback() (/home/chandra/lightning/tests/fuzz/fuzz-init_received+0x1d29a7) (BuildId: 2e1951a325c952a9be03b7b50fae5fe68183b738) #3 0x7a3aa644532f (/usr/lib/x86_64-linux-gnu/libc.so.6+0x4532f) (BuildId: 42c84c92e6f98126b3e2230ebfdead22c235b667) #4 0x7a3aa649eb2b in __pthread_kill_implementation nptl/pthread_kill.c:43:17 #5 0x7a3aa649eb2b in __pthread_kill_internal nptl/pthread_kill.c:78:10 #6 0x7a3aa649eb2b in pthread_kill nptl/pthread_kill.c:89:10 #7 0x7a3aa644527d in raise signal/../sysdeps/posix/raise.c:26:13 #8 0x7a3aa64288fe in abort stdlib/abort.c:79:7 #9 0x7a3aa642881a in __assert_fail_base assert/assert.c:96:3 #10 0x7a3aa643b516 in __assert_fail assert/assert.c:105:3 #11 0x599305e202f5 in del_tree /home/chandra/lightning/ccan/ccan/tal/tal.c:428:2 #12 0x599305e1f93c in tal_free /home/chandra/lightning/ccan/ccan/tal/tal.c:532:3 #13 0x599305b92370 in clean_tmpctx /home/chandra/lightning/common/utils.c:112:3 #14 0x599305c9dad2 in run /home/chandra/lightning/tests/fuzz/fuzz-init_received.c:161:2

Error seems to indicate that a pointer's ownership was previously "taken" by some other function. You'll need to figure out where that's happening to debug further.

The error was apparently in the test's mock for peer_connected(). The function's actual definition "TAKES" the passed their_features parameter, and replicating that behavior in the mock seems to fix the issue.

morehouse

What exactly are we doing in the interceptors? From a quick look, I don't see io_read being called in peer_init_received at all. Please explain in detailed comments so reviewers and maintainers don't need to go searching themselves.

morehouse · 2025-07-07T18:58:27Z

tests/fuzz/fuzz-init_received.c

+static size_t io_buf_pos = 0;
+
+static struct io_plan *
+test_write(struct io_conn *conn, const void *data, size_t len,


Still missing documentation on what exactly test_read and test_write are doing, and why. If I can't figure out what's going on here, probably no one else can either.

Chand-ra · 2025-07-08T08:42:55Z

After some inspection, I noticed that io_read() is only invoked by the function under test, peer_init_received(), when the decrypted message (fuzzer input in our case) is an unknown message of odd type: handled by is_unknown_msg_discardable(msg). In such cases, the function returns to read_init() to read the next message.

On the other hand, io_write() is called only when peer_init_received() encounters an error and needs to send a warning.

Given this, I believe it's acceptable to replace both io_read() and io_write() with simple mocks in the fuzz target, rather than trying to simulate the full I/O behavior.

morehouse

ACK 89067a0.

Tested and verified coverage of peer_init_received.

rustyrussell · 2025-11-10T05:23:30Z

Rebased.

Changelog-None: `peer_init_received()` in `connectd/peer_exchange_initmsg.{c, h}` is responsible for handling `init` messages defined in BOLT ElementsProject#1. Since it deals with untrusted input, add a test for it.

Add a minimal input set as a seed corpus for the newly introduced test. This leads to discovery of interesting code paths faster.

morehouse suggested changes Jun 30, 2025

View reviewed changes

Chand-ra force-pushed the init_received branch from 2408b16 to 2a325a9 Compare July 3, 2025 04:23

morehouse reviewed Jul 7, 2025

View reviewed changes

Chand-ra force-pushed the init_received branch from 2a325a9 to 89067a0 Compare July 8, 2025 08:57

morehouse approved these changes Jul 8, 2025

View reviewed changes

rustyrussell added this to the v25.12 milestone Nov 7, 2025

rustyrussell force-pushed the init_received branch from 89067a0 to 016d6c6 Compare November 10, 2025 05:23

rustyrussell enabled auto-merge (rebase) November 10, 2025 05:23

rustyrussell force-pushed the init_received branch 2 times, most recently from 0df82bd to bde171b Compare November 13, 2025 02:45

Chandra Pratap added 2 commits November 13, 2025 14:53

fuzz-tests: Add a test for peer_init_received()

3698826

Changelog-None: `peer_init_received()` in `connectd/peer_exchange_initmsg.{c, h}` is responsible for handling `init` messages defined in BOLT ElementsProject#1. Since it deals with untrusted input, add a test for it.

fuzz-tests: Add a seed corpus for the new test

97bfc2d

Add a minimal input set as a seed corpus for the newly introduced test. This leads to discovery of interesting code paths faster.

rustyrussell force-pushed the init_received branch from bde171b to 97bfc2d Compare November 13, 2025 04:23

rustyrussell disabled auto-merge November 13, 2025 22:38

rustyrussell merged commit e95e5f9 into ElementsProject:master Nov 13, 2025
36 of 39 checks passed

Conversation

Chand-ra commented Jun 30, 2025

Checklist

Uh oh!

Chand-ra commented Jun 30, 2025

Uh oh!

morehouse left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morehouse left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Chand-ra commented Jul 8, 2025

Uh oh!

morehouse left a comment

Choose a reason for hiding this comment

Uh oh!

rustyrussell commented Nov 10, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants