fuzz-tests: improve `fuzz-bech32` #8311

Chand-ra · 2025-06-02T05:28:23Z

Add improvements to tests/fuzz/fuzz-bech32 and add the coverage increasing inputs to the corresponding corpus.

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

Changelog-None: Use the common library utilities for temporary allocations instead of manually calling `malloc` and `free`. This makes the code conformant with rest of the codebase and reduces the chances of leaks.

According to `common/bech32.h`, the valid values of witness program version are between 0 and 16 (inclusive). Update the test to iterate over all of these values.

morehouse · 2025-06-07T17:27:44Z

tests/fuzz/fuzz-bech32.c

+	uint8_t *converted = tal_arr(tmpctx, uint8_t, size * 2);
+	size_t converted_len = 0;
+	if (bech32_convert_bits(converted, &converted_len, 5, data, size, 8, 1)) {
+		uint8_t *deconverted = tal_arr(tmpctx, uint8_t, converted_len * 2);


Does deconverted need to be size converted_len * 2? If we're going from 5 bits back to 8 bits, shouldn't we need at most size bytes?

morehouse · 2025-06-07T17:30:30Z

tests/fuzz/fuzz-bech32.c

 		assert(memcmp(data_out, data, data_out_len) == 0);
 	}

+	/* Test 8-to-5 bit roundtrip conversion */


Would it make sense to do the roundtrip as part of the above call to bech32_convert_bits?

Makes sense, the calls above don't test a whole lot.

morehouse · 2025-06-11T19:42:54Z

tests/fuzz/fuzz-bech32.c

-	data_out_len = 0;
-	bech32_convert_bits(data_out, &data_out_len, 8, data, size, 5, 0);
+	/* First conversion uses pad=1 to ensure all bits are captured. */
+	if (bech32_convert_bits(data_out, &data_out_len, 5, data, size, 8, 1)) {


IIUC the original code converts 5 bits to 8 bits, while now this converts 8 bits to 5 bits. This change largely invalidates the existing corpus.

Can we keep the same direction of the conversion, to maximize the corpus effectiveness?

The test does perform a 8 to 5 bit conversion later in the code to perform the roundtrip check:

if (bech32_convert_bits(deconv_data_out, &deconv_data_out_len, 8, data_out, data_out_len, 5, 0))

Do you suggest that we should swap the order of the conversions-that is, 5 to 8 first then 8 to 5?

Original Code

bech32_convert_bits(data_out, &data_out_len, 8, data, size, 5, 1);

New Code

if (bech32_convert_bits(data_out, &data_out_len, 5, data, size, 8, 1)) { ... bech32_convert_bits(deconv_data_out, &deconv_data_out_len, 8, data_out, data_out_len, 5, 0); }

Corpus Invalidation

Since the original corpus was created with the original code, it optimizes coverage for the 5-to-8 bit conversion. The new code reverses the direction of the initial conversion, which means all those original corpus inputs are no longer optimized for this fuzz target.

Yes there is still a 5-to-8 bit conversion later on for the round trip, but that doesn't solve the corpus invalidation since the second conversion is done on data_out, not data.

I've pushed a version where we do the 5-to-8 bit conversion first, but we need to first convert the fuzzer input to 5-bit first:

u8 *five_bit_data = tal_dup_arr(tmpctx, u8, data, size, 0); for (size_t i = 0; i < size; i++) five_bit_data[i] &= 0x1F;

if we want to perform a memcmp at the end of the roundtrip. Not sure if this maintains the corpus' validity though.

Yeah, that invalidates the existing corpus. But I think this change makes some sense if we want to do round-trip checks.

morehouse

Since the corpus is invalidated by this change, we should re-minimize it. You can do that by moving all the inputs in corpora/fuzz-bech32 to a temp dir and then merging the temp dir into the now empty corpora/fuzz-bech32 directory.

That should help to remove inputs that no longer improve coverage.

morehouse · 2025-06-24T16:38:00Z

tests/fuzz/fuzz-bech32.c

+		}
+	}
+
+	data_out = tal_arr(tmpctx, uint8_t, size * 2);


Only size bytes are needed.

Suggested change

data_out = tal_arr(tmpctx, uint8_t, size * 2);

data_out = tal_arr(tmpctx, uint8_t, size);

morehouse · 2025-06-24T16:39:53Z

tests/fuzz/fuzz-bech32.c

-	data_out_len = 0;
-	bech32_convert_bits(data_out, &data_out_len, 8, data, size, 5, 0);
+	/* First conversion uses pad=1 to ensure all bits are captured. */
+	if (bech32_convert_bits(data_out, &data_out_len, 5, data, size, 8, 1)) {


Yeah, that invalidates the existing corpus. But I think this change makes some sense if we want to do round-trip checks.

Currently, the test only verifies the 5-to-8 bit conversion. Replace it with a roundtrip check that verifies 8-to-5 bit conversion as well.

Change in the fuzzing scheme of fuzz-bech32 led to the discovery of test inputs that result in greater in code coverage. Add these inputs to the test's seed corpus.

Chand-ra · 2025-06-25T07:00:05Z

Since the corpus is invalidated by this change, we should re-minimize it. You can do that by moving all the inputs in corpora/fuzz-bech32 to a temp dir and then merging the temp dir into the now empty corpora/fuzz-bech32 directory.

That should help to remove inputs that no longer improve coverage.

Done in the latest push.

morehouse

ACK 34f5520.

Chandra Pratap added 2 commits June 2, 2025 05:25

fuzz-tests: Replace manual allocations with tal_arr()

5ed84e9

Changelog-None: Use the common library utilities for temporary allocations instead of manually calling `malloc` and `free`. This makes the code conformant with rest of the codebase and reduces the chances of leaks.

fuzz-tests: Test over all possible witness version values

bfb072e

According to `common/bech32.h`, the valid values of witness program version are between 0 and 16 (inclusive). Update the test to iterate over all of these values.

morehouse reviewed Jun 7, 2025

View reviewed changes

Chand-ra force-pushed the fuzz-bech32 branch 2 times, most recently from ece5cfc to f595374 Compare June 9, 2025 08:38

morehouse reviewed Jun 11, 2025

View reviewed changes

Chand-ra force-pushed the fuzz-bech32 branch from f595374 to 3280f98 Compare June 23, 2025 08:58

morehouse reviewed Jun 24, 2025

View reviewed changes

Chandra Pratap added 2 commits June 25, 2025 06:57

fuzz-tests: test 8-to-5 bit conversion

1e251cf

Currently, the test only verifies the 5-to-8 bit conversion. Replace it with a roundtrip check that verifies 8-to-5 bit conversion as well.

fuzz-tests: Add coverage-increasing inputs to seed corpora

34f5520

Change in the fuzzing scheme of fuzz-bech32 led to the discovery of test inputs that result in greater in code coverage. Add these inputs to the test's seed corpus.

Chand-ra force-pushed the fuzz-bech32 branch from 3280f98 to 34f5520 Compare June 25, 2025 06:59

morehouse approved these changes Jun 27, 2025

View reviewed changes

rustyrussell merged commit 6e16f94 into ElementsProject:master Sep 22, 2025

	data_out = tal_arr(tmpctx, uint8_t, size * 2);
	data_out = tal_arr(tmpctx, uint8_t, size);

fuzz-tests: improve fuzz-bech32 #8311

fuzz-tests: improve fuzz-bech32 #8311

Uh oh!

Conversation

Chand-ra commented Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

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

Original Code

New Code

Corpus Invalidation

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!

Choose a reason for hiding this comment

Uh oh!

Chand-ra commented Jun 25, 2025

Uh oh!

morehouse left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fuzz-tests: improve `fuzz-bech32` #8311

fuzz-tests: improve `fuzz-bech32` #8311

Chand-ra commented Jun 2, 2025 •

edited

Loading