-
Notifications
You must be signed in to change notification settings - Fork 10
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
feat: add some initial cargo-fuzz test for queues #13
Conversation
Hmmm, cool! Also, seems like the fuzz target folder, corpus and artifact folder are not gitignored |
* Simplify value generation, no key needed * Check expected value to pop against what embedded-storage gives us * Keep track of capacity (still not working)
@diondokter I'm trying to have the fuzzer determine whether or not a push() operation should fail due to out of capacity, and for that I need to calculate the expected overhead of the metadata. From what I can infer, the per-item overhead is 6 bytes, but aligned to word boundary, and at the start(?) of each page there is a byte controlling which state the page is in? |
|
* Add convenience function for checking the largest slot available in the queue
@diondokter believe it has found one off-by-one bug in next_free_item (but please check it, see a521750). Another fuzz test to add (which I'd like to add in a follow-up PR) is to take an existing 'image' that may be corrupted in random ways and see what happens. Regarding the distribution, we can do that by using a custom type and implementing the Arbitrary trait. However, I think what's there is pretty good already. Finally, there is a question of semantics and another potential bug. At the moment a push of an empty item will fail with StorageFull in some cases, and some cases not (depending on what happened before). I'd like to get your view on what the behavior should be if there is space for the aligned header but not the empty item. My thinking is that it should be allowed (could be useful as some kind of external marker?) |
Nice! I'll review later. As for the 0 length, that is allowed. It didn't use to, so the weird behavior might be from that still. So yeah, I do consider that a bug |
@diondokter Just a quick update, fixed a bug in the new find_max_fit() handling 0 length correctly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, looks pretty good! Only a couple of small things.
Btw, I just added a simple CI script on master. Not sure how much that would bring us, but at least the fuzzer would run every change, even if it's brief. For statime we do that (though that still uses the old actions-rs): https://github.com/pendulum-project/statime/blob/main/.github/workflows/rust.yml#L101-L124 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a bunch!
I played with using cargo-fuzz for testing the queue behavior. I wanted to reuse the mock flash for that, but that would require gating it behind some hidden feature, not sure if you have a preferred way.