Skip to content
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

New PR with Devin's complete changes #507

Merged
merged 11 commits into from
Dec 17, 2024

Conversation

pmeredit
Copy link
Contributor

This was generated completely by Devin AI with some prompting. Just checking to see what the fuzz results are

@isabelatkinson
Copy link
Contributor

@pmeredit did you want a review on this one?

@pmeredit
Copy link
Contributor Author

@pmeredit did you want a review on this one?

Sorry, not yet! I'm just seeing how these ai generated fuzz tests work. Bson-rust won't evergreen patch for me for some reason.

@@ -154,7 +157,25 @@ functions:
- command: shell.exec
params:
script: |
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These evergreen changes are completely AI written except for the echo statement. Pretty impressive

let utf8_cases = doc! {
"empty": "",
"null_bytes": "hello\0world",
"unicode": "🦀💻🔒",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Devin seems to have developed a sense of humor 😂

@pmeredit pmeredit closed this Dec 12, 2024
@pmeredit pmeredit force-pushed the topic/devin_bson_fuzzing_complete branch from 17654e8 to f1a08f8 Compare December 12, 2024 18:59
@pmeredit
Copy link
Contributor Author

I'm not sure why force pushing to my topic branch closed this PR

@pmeredit
Copy link
Contributor Author

reopen

@@ -13,15 +13,18 @@ stepback: true
command_type: system

# Protect ourself against rogue test case, or curl gone wild, that runs forever
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, devin actually updated this comment

@pmeredit
Copy link
Contributor Author

@isabelatkinson I think it's worth looking at this now. This is almost entirely AI generated.

@abr-egn abr-egn self-requested a review December 16, 2024 15:47
Copy link
Contributor

@abr-egn abr-egn left a comment

Choose a reason for hiding this comment

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

“Motive," the construct said. "Real motive problem, with an AI. Not human, see?"
"Well, yeah, obviously."
"Nope. I mean, it's not human. And you can't get a handle on it. Me, I'm not human either, but I respond like one. See?"
"Wait a sec," Case said. "Are you sentient, or not?"
"Well, it feels like I am, kid, but I'm really just a bunch of ROM. It's one of them, ah, philosophical questions, I guess...." The ugly laughter sensation rattled down Case's spine. "But I ain't likely to write you no poem, if you follow me. Your AI, it just might. But it ain't no way human.”

― William Gibson, Neuromancer

In general, I'm personally going to have a high value bar that AI-generated PRs will need to clear - they need the same sort of focused line-by-line review that a PR from an entirely unknown contributor does, and it's harder to have a conversation about motivation or tradeoffs.

Was the prompt given here on the general level of "add security-focused fuzz tests" and it picked the cases itself, or did it have guidance on which specific areas to focus on?

[package]
name = "bson-fuzz"
version = "0.0.1"
authors = ["Automatically generated"]
publish = false
edition = "2021"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd generally prefer to do edition bumps in their own PRs, but it doesn't look like it had any impact here otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Devin was not pleased that we didn't have an edition 😂

use bson::RawDocument;

fuzz_target!(|buf: &[u8]| {
if buf.len() >= 4 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the length guard? RawDocument::from_bytes should handle the smaller cases fine (and if it doesn't we want to know!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Devin got this from the docs and/or source, I agree it would be better to remove this guard.

RawBsonRef::Double(d) => {
if d.is_nan() {
Some(RawBsonRef::Double(f64::NAN).to_raw_bson())
} else if d.is_infinite() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: this case can be collapsed into the final else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Presumably Devin would fix this kind of issue if asked. You're definitely right. In this case I think I'll fix it myself.

None
}
}
RawBsonRef::ObjectId(id) => Some(RawBsonRef::ObjectId(id).to_raw_bson()),
Copy link
Contributor

Choose a reason for hiding this comment

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

This (and all the other cases with no extra logic) can just be collapsed down to a single

other => Some(other.to_raw_bson()),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is interesting. I often don't like catch-alls because they can cause errors when new types are added, but I don't think the bson spec will be adding types any time soon.

use libfuzzer_sys::fuzz_target;
use std::str::FromStr;

fn convert_bson_ref(bson_ref: RawBsonRef) -> Option<RawBson> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand the intent of this function (or of the fuzz test in this file, for that matter). This function converts from a borrowed to an owned value (e.g. what to_raw_bson does) and along the way does some sort-of validation that duplicates what the bson library does in deserialization. If the goal is to reimplement the validation as defense in depth, I think having a validate method that doesn't do the ownership conversion would be much simpler; if it's to have a fuzz test that serialized bytes match deserialized bytes, I don't think this is needed at all.

The handling of RawBsonRef::Double is particularly confusing. Why construct a fresh instance with f64::NAN in the d.is_nan() case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is what it did when told to compare the input to the output

}
}

#[derive(Debug, Arbitrary)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this an Arbitrary struct rather than the fuzzed [u8] slice the other fuzz targets use?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I rewrote this test entirely. Going to have to say Devin failed on this one.

}
RawBsonRef::Binary(b) if b.subtype == BinarySubtype::Generic => {
// Test UTF-8 validation on binary data
let _ = std::str::from_utf8(b.bytes);
Copy link
Contributor

Choose a reason for hiding this comment

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

This ... isn't really fuzz testing the bson library at this point, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, this is just wrong. Why would a generic binary be utf8?

src/spec/mod.rs Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

As a matter of style, we strongly prefer src/foo.rs over src/foo/mod.rs - makes keeping track of editor tabs easier :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It did this because it added spec/fmt.rs also. I could move that code into spec.rs if you prefer, though?

src/spec/mod.rs Outdated
@@ -23,6 +23,10 @@

use std::convert::From;

mod fmt;
#[allow(unused_imports)]
pub use self::fmt::*;
Copy link
Contributor

Choose a reason for hiding this comment

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

Also as a matter of style, we avoid glob imports. Since all that file had is the one impl, I think that can just get inlined into this mod.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should have read this before my last comment, hah

@pmeredit
Copy link
Contributor Author

“Motive," the construct said. "Real motive problem, with an AI. Not human, see?"
"Well, yeah, obviously."
"Nope. I mean, it's not human. And you can't get a handle on it. Me, I'm not human either, but I respond like one. See?"
"Wait a sec," Case said. "Are you sentient, or not?"
"Well, it feels like I am, kid, but I'm really just a bunch of ROM. It's one of them, ah, philosophical questions, I guess...." The ugly laughter sensation rattled down Case's spine. "But I ain't likely to write you no poem, if you follow me. Your AI, it just might. But it ain't no way human.”

― William Gibson, Neuromancer

In general, I'm personally going to have a high value bar that AI-generated PRs will need to clear - they need the same sort of focused line-by-line review that a PR from an entirely unknown contributor does, and it's harder to have a conversation about motivation or tradeoffs.

Was the prompt given here on the general level of "add security-focused fuzz tests" and it picked the cases itself, or did it have guidance on which specific areas to focus on?

Aha! At first it was "add security-focused fuzz tests", then I talked to it a bit more on the serialize test in particular because its first pass was to simply check that serialize didn't crash, whereas I demand that the output roundtrip with the input.

@pmeredit
Copy link
Contributor Author

@abr-egn I'm also not convinced the malformed_length test is adding any additional benefity, wdyt?

@pmeredit pmeredit requested a review from abr-egn December 17, 2024 15:27
Copy link
Contributor

@abr-egn abr-egn left a comment

Choose a reason for hiding this comment

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

LGTM modulo malformed_length removal.

@abr-egn
Copy link
Contributor

abr-egn commented Dec 17, 2024

@abr-egn I'm also not convinced the malformed_length test is adding any additional benefity, wdyt?

Agreed - the value there is in the corpus generation, there's no need to have a test that just deserializes when that's very solidly covered by all the rest.

@abr-egn abr-egn removed the request for review from isabelatkinson December 17, 2024 20:17
@pmeredit pmeredit merged commit 896a5e1 into mongodb:main Dec 17, 2024
1 check was pending
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.

3 participants