fuzz-tests: Add a test for `codex32` operations #8390

Chand-ra · 2025-07-03T10:52:06Z

Add a test for codex32_encode() and codex32_secret_decode() defined in common/codex32.{c, h}.

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

morehouse · 2025-07-07T19:23:07Z

tests/fuzz/fuzz-codex32.c

+static const char *valid_vectors[] = {
+	"ms10testsxxxxxxxxxxxxxxxxxxxxxxxxxx4nzvca9cmczlw",
+	"MS12NAMEA320ZYXWVUTSRQPNMLKJHGFEDCAXRPP870HKKQRM",
+	"ms13cashsllhdmn9m42vcsamx24zrxgs3qqjzqud4m0d6nln",
+	"ms10leetsllhdmn9m42vcsamx24zrxgs3qqjzqud4m0d6nlnve25gvezzyqqtum9pgv99ycma",
+	"MS100C8VSM32ZXFGUHPCHTLUPZRY9X8GF2TVDW0S3JN54KHCE6MUA7LQPZYGSFJD6AN074RXV"
+	"CEMLH8WU3TK925ACDEFGHJKLMNPQRSTUVWXY06FHPV80UNDVARHRAK"
+};


Nit: I think it's cleaner to put seeds in the corpus directory instead of hard-coding them into the fuzz target.

I'd prefer to just put one seed here, or even get rid of initial_input entirely and just return 0 instead for simplicity.

morehouse · 2025-07-07T19:27:09Z

tests/fuzz/fuzz-codex32.c

+			if (streq(parts->hrp, "ms"))
+				parts->hrp = "MS";
+			else
+				parts->hrp = "ms";
+			break;


Do we need to limit the HRP to "MS" or "ms"? Allowing other mutations may trigger new coverage.

morehouse · 2025-07-07T19:30:59Z

tests/fuzz/fuzz-codex32.c

+		case 1: /* Mutate threshold (0-9) */
+			parts->threshold = rand() % 10;
+			break;


Do we need to limit threshold here? Allowing other values may trigger new coverage.

morehouse · 2025-07-07T19:42:13Z

tests/fuzz/fuzz-codex32.c

+			for (int i = 0; i < 4; i++)
+				parts->id[i] = 'A' + (rand() % 26);


Valid IDs have lowercase bech32 characters only. If the goal is to make a valid ID, we should be using those. Otherwise maybe we should leave it unconstrained?

morehouse · 2025-07-07T19:47:36Z

tests/fuzz/fuzz-codex32.c

+			parts->id[4] = '\0';
+			break;
+		case 3: /* Mutate share index (valid char) */
+			parts->share_idx = "abcdefghijklmnopqrstuvwxyzABCDEF"[rand() % 32];


It seems codex32_secret_encode overwrites the share_idx with s anyway, so this mutation is currently pointless.

morehouse · 2025-07-07T19:56:44Z

tests/fuzz/fuzz-codex32.c

+			if (tal_bytelen(parts->payload) > 0) {
+			size_t mutate_len = 1 + rand() % tal_bytelen(parts->payload);
+			LLVMFuzzerMutate((u8 *) parts->payload, mutate_len,
+					tal_bytelen(parts->payload));
+			}


We should let LLVMFuzzerMutate decide how many bytes to mutate, and we should allow it to increase or decrease the payload size.

Suggested change

if (tal_bytelen(parts->payload) > 0) {

size_t mutate_len = 1 + rand() % tal_bytelen(parts->payload);

LLVMFuzzerMutate((u8 *) parts->payload, mutate_len,

tal_bytelen(parts->payload));

}

size_t cur_size = tal_bytelen(parts->payload);

tal_resize(parts->payload, max_size);

size_t new_size = LLVMFuzzerMutate((u8 *) parts->payload, cur_size, max_size);

tal_resize(parse->payload, new_size);

morehouse · 2025-07-07T20:03:33Z

tests/fuzz/fuzz-codex32.c

+	switch(rand() % 5) {
+		case 0:
+			child->hrp = p2->hrp;
+			break;
+		case 1:
+			child->threshold = p2->threshold;
+			break;
+		case 2:
+			memcpy(child->id, p2->id, 5);
+			break;
+		case 3:
+			child->share_idx = p2->share_idx;
+			break;
+		case 4: /* Payload crossover */
+			child->payload = tal_arr(child, u8, tal_bytelen(p2->payload));
+			memcpy((u8 *) child->payload, p2->payload, tal_bytelen(p2->payload));
+			break;


The idea behind the crossover mutator is to combine pieces of each input into a new input (like genetic crossover). Using the cross_over helper function from libfuzz.h here would greatly strengthen this mutator.

Chand-ra · 2025-07-08T07:16:38Z

Hey @morehouse, I was working on incorporating the feedback above but the resulting target fails with an out-of-bounds error when executed on the current corpus. Executing the resulting crash file however, fails to reproduce the crash:

chand@Ubuntu:~/lightning$ tests/fuzz/fuzz-codex32 local_corpora/fuzz-codex32/

INFO: found LLVMFuzzerCustomMutator (0x5f37a5e196d0). Disabling -len_control by default.
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 202808252
INFO: Loaded 1 modules   (35545 inline 8-bit counters): 35545 [0x5f37a621f610, 0x5f37a62280e9), 
INFO: Loaded 1 PC tables (35545 PCs): 35545 [0x5f37a62280f0,0x5f37a62b2e80), 
INFO:      117 files found in local_corpora/fuzz-codex32/
INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 4096 bytes
INFO: seed corpus: files: 117 min: 10b max: 540b total: 14861b rss: 42Mb
#118	INITED cov: 252 ft: 542 corp: 41/4581b exec/s: 0 rss: 46Mb
#119	NEW    cov: 252 ft: 585 corp: 42/4708b lim: 4096 exec/s: 0 rss: 47Mb L: 127/262 MS: 1 CustomCrossOver-
#120	NEW    cov: 252 ft: 603 corp: 43/4809b lim: 4096 exec/s: 0 rss: 47Mb L: 101/262 MS: 2 InsertByte-Custom-
#131	NEW    cov: 252 ft: 604 corp: 44/4895b lim: 4096 exec/s: 0 rss: 47Mb L: 86/262 MS: 1 CustomCrossOver-
#157	NEW    cov: 252 ft: 618 corp: 45/5022b lim: 4096 exec/s: 0 rss: 47Mb L: 127/262 MS: 1 CustomCrossOver-
#194	NEW    cov: 252 ft: 619 corp: 46/5056b lim: 4096 exec/s: 0 rss: 48Mb L: 34/262 MS: 3 CMP-Custom-CustomCrossOver- DE: "\377\377\377\377"-
common/codex32.c:119:28: runtime error: index 242 out of bounds for type 'const uint8_t[32]' (aka 'const unsigned char[32]')
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior common/codex32.c:119:28 
MS: 1 ShuffleBytes-; base unit: c00e8766131642fffb333d9c1e3f9170c31c1b1d

artifact_prefix='./'; Test unit written to ./crash-da39a3ee5e6b4b0d3255bfef95601890afd80709
Base64: 

chand@Ubuntu:~/lightning$ tests/fuzz/fuzz-codex32 crash-da39a3ee5e6b4b0d3255bfef95601890afd80709

INFO: found LLVMFuzzerCustomMutator (0x601ae86816d0). Disabling -len_control by default.
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 4227437988
INFO: Loaded 1 modules   (35545 inline 8-bit counters): 35545 [0x601ae8a87610, 0x601ae8a900e9), 
INFO: Loaded 1 PC tables (35545 PCs): 35545 [0x601ae8a900f0,0x601ae8b1ae80), 
tests/fuzz/fuzz-codex32: Running 1 inputs 1 time(s) each.
Running: crash-da39a3ee5e6b4b0d3255bfef95601890afd80709
Executed crash-da39a3ee5e6b4b0d3255bfef95601890afd80709 in 5 ms
***
*** NOTE: fuzzing was not performed, you have only
***       executed the target code on a fixed set of inputs.
***

chand@Ubuntu:~/lightning$

Is this an actual bug or something wrong with the target? Any ideas on how I can investigate this further?

morehouse · 2025-07-08T17:06:01Z

Since it doesn't reproduce, it's likely a bug in one of your custom mutators. Check the line of code that UBSan is pointing to and go from there.

Chand-ra · 2025-07-09T06:51:18Z

The test works without breaking now.

morehouse · 2025-07-09T16:03:39Z

tests/fuzz/fuzz-codex32.c

+		struct codex32 *mutable_parts = codex32_dup(tmpctx, parts);
+		tal_free(parts);
+		parts = mutable_parts;


Why is the copy needed, especially since we just discard the original anyway?

morehouse · 2025-07-09T16:17:49Z

tests/fuzz/fuzz-codex32.c

+			for (int i = 0; i < 4; i++)
+				parts->id[i] = rand();
+			parts->id[4] = '\0';
+			break;


I think we should prefer LLVMFuzzerMutate here.

Suggested change

for (int i = 0; i < 4; i++)

parts->id[i] = rand();

parts->id[4] = '\0';

break;

size_t id_len = sizeof(parts->id) - 1;

LLVMFuzzerMutate(parts->id, id_len, id_len);

parts->id[id_len] = '\0';

break;

morehouse · 2025-07-09T16:24:54Z

tests/fuzz/fuzz-codex32.c

+		case 3: /* Mutate payload */
+			{
+				size_t cur_size = tal_bytelen(parts->payload);
+				size_t max_payload_size = max_size;


max_payload_size is unnecessary; we can just use max_size directly.

morehouse · 2025-07-09T16:26:28Z

tests/fuzz/fuzz-codex32.c

+			{
+				size_t cur_size = tal_bytelen(parts->payload);
+				size_t max_payload_size = max_size;
+				u8 *new_payload = tal_arr(parts, u8, max_payload_size);


Can't we just resize the original parts->payload, rather than managing an extra pointer?

morehouse · 2025-07-09T16:34:15Z

tests/fuzz/fuzz-codex32.c

+								parts->threshold, parts->payload,
+								tal_bytelen(parts->payload), &reencoded);
+			if (!err) {
+				size_t len = strlen(reencoded);


If we can use tal_bytelen here instead, that should be more efficient.

morehouse · 2025-07-09T16:43:11Z

tests/fuzz/fuzz-codex32.c

+				/* Apply mutation */
+				size_t new_size = LLVMFuzzerMutate(new_payload,
+							cur_size < max_payload_size ?
+							cur_size : max_payload_size,
+							max_payload_size);


Not sure why we would need to take the min here.

Suggested change

/* Apply mutation */

size_t new_size = LLVMFuzzerMutate(new_payload,

cur_size < max_payload_size ?

cur_size : max_payload_size,

max_payload_size);

/* Apply mutation */

size_t new_size = LLVMFuzzerMutate(new_payload,

cur_size,

max_payload_size);

morehouse · 2025-07-09T16:52:36Z

tests/fuzz/fuzz-codex32.c

+				u8 new_id[5];
+				cross_over((const u8 *)p1->id, 4, (const u8 *)p2->id, 4,
+					   new_id, 4, rand());
+				new_id[4] = '\0';
+				memcpy(child->id, new_id, 4);


A few issues with this code.

we fail to copy the null byte

we can make it simpler by mutating child->id directly.

Suggested change

u8 new_id[5];

cross_over((const u8 *)p1->id, 4, (const u8 *)p2->id, 4,

new_id, 4, rand());

new_id[4] = '\0';

memcpy(child->id, new_id, 4);

size_t id_len = sizeof(p1->id) - 1;

cross_over((const u8 *)p1->id, id_len, (const u8 *)p2->id, id_len,

child->id, id_len, rand());

child->id[id_len] = '\0';

morehouse · 2025-07-09T16:56:58Z

tests/fuzz/fuzz-codex32.c

+				u8 *new_payload = tal_arr(child, u8, max_out_size);
+				size_t new_payload_len = cross_over(p1->payload, p1_len,
+								p2->payload, p2_len,
+								new_payload, max_out_size, rand());
+				tal_free(child->payload);
+				child->payload = new_payload;


Why not resize child->payload for simplicity?

Suggested change

u8 *new_payload = tal_arr(child, u8, max_out_size);

size_t new_payload_len = cross_over(p1->payload, p1_len,

p2->payload, p2_len,

new_payload, max_out_size, rand());

tal_free(child->payload);

child->payload = new_payload;

tal_resize(&child->payload, u8, max_out_size);

size_t new_payload_len = cross_over(p1->payload, p1_len,

p2->payload, p2_len,

child->payload, max_out_size, rand());

morehouse · 2025-07-09T16:57:53Z

tests/fuzz/fuzz-codex32.c

+			if (rand() % 2)
+				child->threshold = p2->threshold;
+			if (rand() % 2)
+				memcpy(child->id, p2->id, 5);


Nit

Suggested change

memcpy(child->id, p2->id, 5);

memcpy(child->id, p2->id, sizeof(p2->id));

morehouse · 2025-07-09T17:00:12Z

tests/fuzz/fuzz-codex32.c

+			}
+			break;
+
+		case 4: /* Random combination */


If we want to do a random combination, perhaps we should create functions for the above crossover cases, so we can reuse them here.

I think that would make the test more complicated without the added benefit to match, perhaps we should get rid of this case altogether?

Changelog-None: Add a test for `codex32_encode()` and `codex32_secret_decode()` defined in `common/codex32.{c, h}`.

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 Jul 7, 2025

View reviewed changes

Chand-ra force-pushed the codex32 branch from 2900b02 to 94c205f Compare July 8, 2025 07:17

Chand-ra force-pushed the codex32 branch from 94c205f to e8efff3 Compare July 9, 2025 06:50

morehouse suggested changes Jul 9, 2025

View reviewed changes

Chandra Pratap added 2 commits July 11, 2025 07:20

fuzz-tests: Add a test for codex32 operations

c0b42ce

Changelog-None: Add a test for `codex32_encode()` and `codex32_secret_decode()` defined in `common/codex32.{c, h}`.

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

0ee9574

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

Chand-ra force-pushed the codex32 branch from e8efff3 to 0ee9574 Compare July 11, 2025 07:21

		for (int i = 0; i < 4; i++)
		parts->id[i] = 'A' + (rand() % 26);

	memcpy(child->id, p2->id, 5);
	memcpy(child->id, p2->id, sizeof(p2->id));

fuzz-tests: Add a test for codex32 operations #8390

Are you sure you want to change the base?

fuzz-tests: Add a test for codex32 operations #8390

Conversation

Chand-ra commented Jul 3, 2025

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Chand-ra commented Jul 8, 2025

Uh oh!

morehouse commented Jul 8, 2025

Uh oh!

Chand-ra commented Jul 9, 2025

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!

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!

Uh oh!

fuzz-tests: Add a test for `codex32` operations #8390

fuzz-tests: Add a test for `codex32` operations #8390