chore: add fuzzing across the codebase by acud · Pull Request #5602 · ethersphere/bee

acud · 2026-09-08T13:13:12Z

Description

Adds fuzz testing across the codebase and fixes a few paths that needed patching.

AI Disclosure

This PR contains code that has been generated by an LLM.
I have reviewed the AI generated code thoroughly.
I possess the technical expertise to responsibly review the code generated in this PR.

Co-authored-by: Alok Nerurkar <alok@no-reply.com>

martinconic · 2026-09-17T11:42:52Z

@@ -147,8 +148,14 @@ func New(ctx context.Context, g storage.Getter, putter storage.Putter, address s
 // A Joiner provides Read, Seek and Size functionalities.
 func NewJoiner(ctx context.Context, g storage.Getter, putter storage.Putter, address swarm.Address, rootChunk swarm.Chunk) (file.Joiner, int64, error) {
 	chunkData := rootChunk.Data()


should we also check rootChunk if nil ?

not sure. if yes, we should also check: context not nil, storage.Getter not nil, putter not nil, address not nil. my assumption here is that this is not needed because if such problem exists you will see if very early in a panic etc. and a nil chunk is technically not possible imo

martinconic · 2026-09-17T11:51:34Z

+	pool := make([]byte, swarm.ChunkWithSpanSize*2)
+	f.Add(append(inter, pool...))
+
+	f.Fuzz(func(t *testing.T, data []byte) {


test encrypted references (64 bytes) alongside standard 32-byte references:

- f.Fuzz(func(t *testing.T, data []byte) { + f.Fuzz(func(t *testing.T, data []byte, encryptedRef bool) { ....

martinconic · 2026-09-17T11:56:09Z

+	// the fields are copied into fixed size windows; an oversized field would
+	// either silently overwrite a neighbouring field or, for the value, index
+	// out of range.
+	if len(value) > 32 || len(b.ID) > 32 || len(b.Owner) > 20 {


Checking len(b.ID) > 32 and len(b.Owner) > 20 allows short byte slices. A 10-byte ID or 12-byte Owner is copied into the binary output left-aligned. Deserialization slices 32 and 20 bytes respectively, which mutates the data.
Should we have: if len(value) > 32 || len(b.ID) != 32 || len(b.Owner) != 20 { ?

martinconic · 2026-09-17T11:58:35Z

 func (b *Batch) MarshalBinary() ([]byte, error) {
-	out := make([]byte, 95)
-	copy(out, b.ID)
+	if b.Value == nil {


If b.Value is negative, big.Int.Bytes() serializes the absolute magnitude, and deserialization recovers it as a positive number

how can it be negative?

martinconic · 2026-09-17T11:59:17Z

+	if len(value) > 32 || len(b.ID) > 32 || len(b.Owner) > 20 {
+		return nil, ErrBatchInvalid
+	}
+	out := make([]byte, batchSize)


Should we also have this checks?

if b.BucketDepth > b.Depth || b.Depth > swarm.MaxPO { return nil, ErrBatchInvalid }

martinconic · 2026-09-17T12:00:42Z

 	b.Start = binary.BigEndian.Uint64(buf[64:72])
 	b.Owner = buf[72:92]
 	b.BucketDepth = buf[92]
 	b.Depth = buf[93]


Same check here?

if b.BucketDepth > b.Depth || b.Depth > swarm.MaxPO { return ErrBatchInvalid }

martinconic · 2026-09-17T12:06:45Z

+	// maxScryptMem bounds the memory scrypt.Key is allowed to allocate for a
+	// keyfile supplied set of parameters (it allocates 128*N*r bytes), so that
+	// a malformed or hostile keyfile cannot exhaust the node's memory.
+	maxScryptMem = 1 << 30


AI:
maxScryptP = 256 and maxScryptMem = 1 GiB. This permits hostile keyfiles to demand 256 sequential passes and up to 1 GiB of RAM, causing severe CPU exhaustion or OOM-killing low-memory nodes.

maxScryptMem = 1 << 30

maxScryptMem = 256 * (1 << 20) // 256 MiB: matches standard Ethereum N=262144, r=8
// maxScryptP bounds the parallelization factor, which drives both the
// number of sequential smix passes and the size of the pbkdf2 block.

maxScryptP = 1 << 8

maxScryptP = 1 // Ethereum keystores universally use p=1

this presupposes that:

the node's machine has been hacked

the attacker forged a new keyfile and has put it into the node keyfile directory

?

aloknerurkar

Looks good overall apart from the minor comments. Main question is why we haven't ported the fuzz seed corpus files and if we should add this to CI for regression instead of relying on developers to run this themselves.

aloknerurkar · 2026-09-22T09:45:01Z

+	fi
+
+# Replay the seed corpus and the regression fixtures committed under
+# testdata/fuzz/ without generating new inputs.


The seed corpus is missing in this PR? Without the seed corpus it only replays the in-line f.Add seeds and none of the values that actually caused the crashes.

Also I feel this should be enforced in CI. Maybe with a lower FUZZ_TIME?

i removed them and converted them into actual regression tests. it doesn't make sense to include them because it just adds another level of abstraction on top of the fuzzer because it just seeds the fuzzer with the input needed to deterministically generate the test case inputs. so in other words, it is another layer of abstraction that makes you a regression test on the fly, nothing more. so they were converted into test vectors instead.

aloknerurkar · 2026-09-22T09:49:12Z

+// FuzzFeedPostHandler drives arbitrary HTTP feed creation requests through the feed
+// router path, testing the control-flow bug (CF-01) where error handling in feed creation
+// must never fall through to dereference a nil manifest.
+func FuzzFeedPostHandler(f *testing.F) {


NewDefaultManifest only returns an error for ErrInvalidManifestType, and it is called with the hardcoded DefaultManifestType constant. So the error branch at pkg/api/feed.go:238 is unreachable through this HTTP path regardless of what the fuzzer sends for owner/topic. So not sure if the target provides any coverage of the bug it is documented as targeting.

pkg/api/feed.go:L238-L247 writes an HTTP response in the error branch but omits the return, falling through to feedManifest.Add(...) on a nil manifest. The branch is unreachable, but can become live if someone refactors the manifest pkg.

Either port the one-line return fix along with this target, or drop the CF-01 claim from this comment so it does not read as coverage that exists.

aloknerurkar · 2026-09-22T09:51:59Z

 	wg.Wait()
 }
+
+func TestCurrentRatesConcurrentWithUpdatesSync(t *testing.T) {


Worth a comment on the test saying it is a no-op without -race, so nobody (including AI) later "fixes" it by adding a meaningless assertion or deletes it as dead weight.

aloknerurkar · 2026-09-22T09:55:49Z

+// isolating the target on the pre-signature validation arithmetic. An empty
+// payout models a peer cheque whose JSON omitted CumulativePayout (decoding to a
+// nil *big.Int) — exactly what the swap handler forwards after json.Unmarshal.
+func FuzzReceiveCheque(f *testing.F) {


This test has no assertions at all. This matches the original intent (NIL-06) but it does mean this target cannot catch a logic regression, e.g. ReceiveCheque silently accepting a cheque it should reject. An assertion that a nil/ malformed cheque produces a non-nil error would make this meaningfully stronger.

The seed this is not ported is what made this target deterministic rather than probabilistic.

github-actions Bot assigned acud Sep 8, 2026

Elad Nachmias and others added 2 commits September 14, 2026 07:51

chore: add fuzzing across the codebase

f442828

Co-authored-by: Alok Nerurkar <alok@no-reply.com>

chore: reset linters

852a14f

acud force-pushed the fuzzings branch from e4584e9 to 852a14f Compare September 14, 2026 13:51

acud marked this pull request as ready for review September 16, 2026 19:29

martinconic reviewed Sep 17, 2026

View reviewed changes

martinconic requested review from akrem-chabchoub, aloknerurkar, gacevicljubisa, janos and sbackend123 September 17, 2026 12:09

aloknerurkar reviewed Sep 22, 2026

View reviewed changes

chore: address pr comments

9767536

aloknerurkar approved these changes Sep 23, 2026

View reviewed changes

ethersphere deleted a comment from akrem-chabchoub Sep 23, 2026

chore: address fuzz test

abf290e

Conversation

acud commented Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

AI Disclosure

Uh oh!

Uh oh!

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!

acud Sep 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aloknerurkar 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

acud commented Sep 8, 2026 •

edited

Loading

acud Sep 22, 2026 •

edited

Loading