Re-implement fuzzing via libfuzzer by pedrodesu · Pull Request #658 · servo/rust-smallvec

pedrodesu · 2026-09-22T06:41:07Z

Closes #657.

This might need some additional work; I'm treating this PR more like a draft.
Additionally, I've already ran this fuzzer myself and it has already shown quickly that we have some violations occuring with our current code on v2 (this assumes that the current fuzzing implementation is proper, which I believe it is), namely on try_grow's assert!(new_capacity >= len); and on the fuzzing test's assertion that assert_eq!(small_vec.spilled(), small_vec.capacity() > N);. From what I saw, it's not really failing anywhere else. This also means, of course, that the fuzzing action should naturally fail at this point (I've also changed the fuzzing CI, I think it's working).

Small disclaimer: I've done this work through the small hours of the morning lol

pedrodesu · 2026-09-22T06:50:35Z

Ah, this refactor not only made the implementation much more robust and simple, both in terms of code and running (via libFuzz and cargo-fuzz), but it also improves on the further maintainability of the code and on the actual fuzzing procedure, namely the biggest win being that we now not only check if executing operations panic/crash, but we also test differences between SmallVec and std's Vec.

pedrodesu · 2026-09-22T06:59:42Z

I also don't think it's the best idea to do either fuzzing or benchmarking on PRs. It takes a considerable amount of time (namely for the benchmarking part), the fuzzing is flaky/inconsistent, especially for a 30 secs run, and the benchmarking is probably noisy and unreliable given that it's running on GitHub Actions runners, where it probably shouldn't run. I feel like we should run an extensive, longer, proper fuzzing under a cron schedule and benchmark manually via comments or using dedicated runners instead. But this is an off-topic thing I noticed during the nightly action run and, if relevant, should probably have an issue of its own.

alejandro-vaz · 2026-09-22T12:10:32Z

also, there is a weird interaction with #656 which is not nice at all, the API there is a bit fucked up currently

… requested changes

alejandro-vaz

above

alejandro-vaz · 2026-09-22T17:54:31Z

how do we now interpret the weird fuzzing error in https://github.com/servo/rust-smallvec/actions/runs/35758316781/job/106849613954??

I'm not sure what debug has to do there

pedrodesu · 2026-09-22T18:11:59Z

how do we now interpret the weird fuzzing error in https://github.com/servo/rust-smallvec/actions/runs/35758316781/job/106849613954??

I'm not sure what debug has to do there

That's just the Debug output of the input byte sequence generated by libFuzzer when the target crashed.
What actually failed is written earlier:

thread '<unnamed>' (10010) panicked at src/lib.rs:892:9:
assertion failed: new_capacity >= len

The fuzzer generated a sequence of operations that forced SmallVec into a state where new_capacity dropped below len, triggering an assertion failure.

You should find that failure locally by fuzzing in your machine and accessing the crash file like the output of the action suggests. We could also change the CI action so that if fuzzing fails it gives us the crash either via the action's artifacts or by printing it directly in the fail. That's probably a good idea. As of now our fuzzing action tells us if something fucks up, but we can't actually access the crash to reproduce it ourselves in our machines. But as I said at the start of the issue, the fuzzing seems to fail always on the same two situations, so you can probably get the fail and access the crash yourself by simply running the fuzzing in your machine as of now.

alejandro-vaz · 2026-09-22T21:27:36Z

how is that even possible

has any invariant been broken??

pedrodesu · 2026-09-22T22:20:59Z

Apparently, yes. The invariant that a len, or a to-be-formed, would-be len, <= cap, concretely. But I haven't looked too deep into the panic yet and I'm not on the computer right now. But the way that I wrote the fuzzer is to, among other things, verify invariants. This is the case assuming that, of course, this isn't a false positive from the fuzzer, which it doesn't seem like it is and shouldn't be if the fuzzing is properly done, which it seems like it is.

I will look into this at my earliest convenience. If we can indeed prove that it's a code problem, then this fuzzer is already finding existing problems that we hadn't caught and it should be a good idea to merge. Independently of merging then directly or not, the end goal is to, of course, let the fuzzer run its course for a lot of time without finding any crash or panic.

alejandro-vaz · 2026-09-22T22:58:28Z

great I guess...

I already doubt many parts of our implementations and I'm planning to drop soon an issue group for 10-15 issues regarding code revision, this seems to confirm it

I wonder what would the fuzzer find if ported to v1, and I think we should do it if possible

go investigate the crash whenever you have time if you want, if you don't find time I'll take it no worries

alejandro-vaz · 2026-09-23T12:26:06Z

+    assert_eq!(
+        small_vec.as_slice(),
+        std_vec.as_slice(),
+        "`as_slice()` mismatch"
+    );
+    assert_eq!(
+        small_vec.as_mut_slice(),
+        std_vec.as_mut_slice(),
+        "`as_mut_slice()` mismatch"
+    );


either is unnecessary

if the immutable references are equal, the mutable references will as well

I'd only keep the immutable one honestly

The thing is - they will both be equal in theory. In theory, all of the assertions done in fuzzing should pass, because all of them attempt to guarantee that each invariant isn't violated. as_mut_slice and as_slice, albeit very similar in their internal implementation, are different functions. What happens if in the future we change or try to change one's implementation but don't change the other? What if one of the invariants is ever broken and we don't catch it? What do we lose by asserting that both match? We do lose a bit of certainty by asserting that only one matches. Does this make sense? My point is, we probably shouldn't "stop doing an assertion" because we can imply that it passes from other assertion or because it's "simply logical". Apparently, it's also "simply logical" that cap would always be greater or equal to length, but the previous failing fuzzing output shows that that's apparently not the case.

tl;dr I'd insist we keep as_mut_slice, indexing, get and bounds check. By definition they all pass, until they don't and we have edge cases with UB or crashing. We lose nothing by keeping these assertions. Still, if you'd still prefer to remove them, I can change the code in that manner.

in the case they stop doing what they are meant to do, tests will break before fuzzing matters

honestly I get the point, but they are so low-level invariants that at this point we may as well check that taggedlen isn't lying about the heap invariant. it's like, if this fails then everything else will fail before so it really makes no sense to explicitly check it

alejandro-vaz · 2026-09-23T12:27:31Z

+        assert_eq!(
+            small_vec[i], std_vec[i],
+            "`small_vec[{i}]` doesn't match `std_vec[{i}]`"
+        );


this is unnecessary

if both immutable slices are equal as checked above, all elements are equal

Please reply to the comment above.

alejandro-vaz · 2026-09-23T12:27:40Z

+        assert_eq!(
+            small_vec.get(i),
+            std_vec.get(i),
+            "`small_vec.get({i})` doesn't match `std_vec.get({i})`"
+        );


Please reply to the comment above.

alejandro-vaz · 2026-09-23T12:28:02Z

+    assert_eq!(
+        small_vec.get(small_vec.len()),
+        None,
+        "out-of-bounds `get()` did not return `None`"
+    );


this by definition will always pass

Please reply to the comment above.

alejandro-vaz · 2026-09-23T12:37:36Z

also, CI is complaining that we renamed smallvec_ops.rs to main.rs without doing the proper rename elsewhere

pedrodesu · 2026-09-23T13:09:28Z

Yes, it's in the rust.yml. Fixing that.

pedrodesu · 2026-09-23T13:24:29Z

Btw, I think I figured out the failure that the last CI was giving. We have assert!(new_capacity >= len); on a public function (try_grow). new_capacity is an argument. My guess is that the fuzzer most definitely called the function try_grow via grow via Op::Grow with a new_capacity smaller than len, which is why this panic happened. What stops anyone from replicating this? My guess is that we shouldn't panic/assert, but rather do nothing or return an Err, as try_grow is already supposed to (as it returns a Result).
Additionally, since this function is public, why do we mark it as #[cold]?

pedrodesu · 2026-09-23T13:28:40Z

Indeed, I patched the function up quickly by returning a dummy Ok(()) if new_capacity < len (Of course we should probably return an Err instead or something similar if we do indeed stop the panic) and the fuzzing doesn't seem to complain anymore about this case. At least by running locally, I observe that it's only complaining about the other assertion:

assertion `left == right` failed: `spilled()` doesn't equal to `capacity() > N`
  left: true
 right: false

alejandro-vaz · 2026-09-23T13:28:54Z

I honestly can't reply to that, who knows, it was already there when I came here

I'm going to open full code revision soon because it doesn't make much sense to me either

I'm not exactly sure what it should do but probably not return another error. either don't do anything or make it a precondition or something like that

alejandro-vaz · 2026-09-23T13:30:45Z

Indeed, I patched the function up quickly by returning a dummy Ok(()) if new_capacity < len (Of course we should probably return an Err instead or something similar if we do indeed stop the panic) and the fuzzing doesn't seem to complain anymore about this case. At least by running locally, I observe that it's only complaining about the other assertion:
assertion `left == right` failed: `spilled()` doesn't equal to `capacity() > N`
  left: true
 right: false

okay... why...??

pedrodesu · 2026-09-23T13:37:03Z

I don't know yet. From what I ran, the fuzzing complained about these two specific cases. the try_grow one seems to be discovered and ready to be solved, and it's honestly less worrying because it's a simple assert being fired that we wrote ourselves in the codebase. This one, however, is apparently triggered by an invariant violation via an assertion in the fuzzer itself. I don't know, I'll try to replicate it and understand why this is happening. In the meantime, I'll push the agreed upon changes (Note: I didn't touch the try_grow function in this change. It's quite possible that the fuzzer will complain about it again as it did before.)

…t invariant assertions on fuzzing

Signed-off-by: Pedro Nobre <me@pedrodesu.xyz>

alejandro-vaz · 2026-09-23T14:03:46Z

I agree but in order to make sure CI is happy let's simply comment out those assertions, we'll re-enable them as we fix root causes

alejandro-vaz · 2026-09-23T17:42:27Z

and conflicts with the workflow have appeared, rebase is needed

pedrodesu · 2026-09-23T18:18:40Z

I agree but in order to make sure CI is happy let's simply comment out those assertions, we'll re-enable them as we fix root causes

Wdym? To comment out the assertions that lead to these two fails? Isn't it better to solve them or to even merge with the fuzzing action failure? The fuzzing itself seems to be working, but these fails should be fixed.

alejandro-vaz · 2026-09-23T19:16:27Z

since it's fuzzing I'll let it pass, but I don't think it's good to merge things that fail CI

can you rebase it??

pedrodesu · 2026-09-23T19:25:13Z

I agree with that generally. I think this is a good exception as it's something we just (re)introduced and that is apparently working, as the failing action is just a byproduct.

I can't work on it right now, I should be able to rebase it and perhaps investigate the remaining problem by tomorrow.

port the new fuzzer introduced with servo#658 in v2 to the v1 branch, it should be a copy-paste + update CI if the fuzzers finds any inconsistencies, it's ...

refactor: re-implement fuzzing via libfuzzer

5457fa5

alejandro-vaz self-requested a review September 22, 2026 09:53

alejandro-vaz assigned pedrodesu Sep 22, 2026

alejandro-vaz requested changes Sep 22, 2026

View reviewed changes

alejandro-vaz mentioned this pull request Sep 22, 2026

ci: new CI #644

Merged

alejandro-vaz mentioned this pull request Sep 22, 2026

feat: new allocator features #656

Merged

refactor: added fuzz package to the workspace; implemented some small…

9a5b995

… requested changes

alejandro-vaz requested changes Sep 22, 2026

View reviewed changes

Comment thread fuzz/Cargo.toml

alejandro-vaz approved these changes Sep 22, 2026

View reviewed changes

Comment thread fuzz/fuzz_targets/main.rs

alejandro-vaz requested changes Sep 22, 2026

View reviewed changes

refactor: change fuzz file name

49c6685

alejandro-vaz requested changes Sep 23, 2026

View reviewed changes

Comment thread fuzz/fuzz_targets/main.rs Outdated

alejandro-vaz requested changes Sep 23, 2026

View reviewed changes

pedrodesu added 2 commits September 23, 2026 14:38

refactor: change fuzz bin on ci; make resize a tuple; remove redundan…

ce7b5a6

…t invariant assertions on fuzzing

Merge branch 'v2' into new-fuzzing

c9a4047

Signed-off-by: Pedro Nobre <me@pedrodesu.xyz>

alejandro-vaz mentioned this pull request Sep 23, 2026

port the new fuzzer to v1 #663

Open

chenzeyan54-commits mentioned this pull request Sep 23, 2026

fix: resolve #663 port the new fuzzer to v1 #664

Closed

2 tasks

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

pedrodesu commented Sep 22, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alejandro-vaz commented Sep 22, 2026

Uh oh!

Uh oh!

Uh oh!

alejandro-vaz left a comment

Choose a reason for hiding this comment

Uh oh!

alejandro-vaz commented Sep 22, 2026

Uh oh!

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

Uh oh!

Uh oh!

alejandro-vaz commented Sep 22, 2026

Uh oh!

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

Uh oh!

Uh oh!

alejandro-vaz commented Sep 22, 2026

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!

alejandro-vaz commented Sep 23, 2026

Uh oh!

pedrodesu commented Sep 23, 2026

Uh oh!

pedrodesu commented Sep 23, 2026

Uh oh!

pedrodesu commented Sep 23, 2026

Uh oh!

alejandro-vaz commented Sep 23, 2026

Uh oh!

alejandro-vaz commented Sep 23, 2026

Uh oh!

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

Uh oh!

Uh oh!

alejandro-vaz commented Sep 23, 2026

Uh oh!

alejandro-vaz commented Sep 23, 2026

Uh oh!

pedrodesu commented Sep 23, 2026

Uh oh!

alejandro-vaz commented Sep 23, 2026

Uh oh!

pedrodesu commented Sep 22, 2026 •

edited

Loading

pedrodesu commented Sep 22, 2026 •

edited

Loading

pedrodesu commented Sep 22, 2026 •

edited

Loading

pedrodesu commented Sep 22, 2026 •

edited

Loading

pedrodesu commented Sep 23, 2026 •

edited

Loading