Skip to content

fuzz-tests: improve fuzz-bech32 #8311

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Chand-ra
Copy link

@Chand-ra Chand-ra commented Jun 2, 2025

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

Chandra Pratap added 2 commits June 2, 2025 05:25
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.
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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

@@ -60,5 +60,18 @@ void run(const uint8_t *data, size_t size)
assert(memcmp(data_out, data, data_out_len) == 0);
}

/* Test 8-to-5 bit roundtrip conversion */
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

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.
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)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants