Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Error when deserializing into &str slice in release mode #67

Closed
george-lim opened this issue Oct 13, 2023 · 5 comments · Fixed by #68
Closed

Error when deserializing into &str slice in release mode #67

george-lim opened this issue Oct 13, 2023 · 5 comments · Fixed by #68

Comments

@george-lim
Copy link

george-lim commented Oct 13, 2023

Hey! I created a MRE for a bug I encountered below. This code compiles and works in debug mode, but in release the FooStr struct will not successfully deserialize.

use simd_json_derive::Deserialize;

#[derive(Deserialize)]
struct FooString {
    foo: String,
}

#[derive(Deserialize)]
struct FooStr<'de> {
    foo: &'de str,
}

fn main() {
    // This works fine in both debug and release mode (opt-level = 3)
    let mut json = br#"{"foo":"bar"}"#.to_vec();
    println!("{}", FooString::from_slice(&mut json).unwrap().foo);

    // This works fine in debug but fails in release mode (opt-level = 3)
    let mut json = br#"{"foo":"bar"}"#.to_vec();
    println!("{}", FooStr::from_slice(&mut json).unwrap().foo);
}
@Licenser
Copy link
Member

Hi,

good catch! I added a test:

#[test]
fn foo_str() {
    #[derive(Deserialize)]
    struct FooStr<'de> {
        foo: &'de str,
    }
    let mut json = br#"{"foo":"bar"}"#.to_vec();
    let res = FooStr::from_slice(&mut json);
    assert!(res.is_ok());
    assert_eq!(res.unwrap().foo, "bar");
}

Clearly a bug, not a panic however, it returns a Err the panic comes from the unwrap() I'll rename the issue and see how quick I can fix it :)

@Licenser Licenser changed the title Panic when deserializing string slice in release mode Error when deserializing into &str slice in release mode Oct 13, 2023
@george-lim
Copy link
Author

oh yeah whoops I wasn't paying attention when I wrote the MRE, just threw unwrap for convenience haha

@Licenser
Copy link
Member

This is a really tricky one, I can reproduce it but I can't make sense of it so far

@george-lim
Copy link
Author

george-lim commented Oct 14, 2023

Not sure if this helps, but I found out that the following works in release mode

#[derive(Deserialize)]
struct FooOptionStr<'de> {
    foo: Option<&'de str>,
}

@Licenser Licenser mentioned this issue Oct 14, 2023
@Licenser
Copy link
Member

Licenser commented Oct 14, 2023

Okay, this took a lot of digging, but I found it. The use of MaybeUninit and .assume_init() was wrong and led to undefined behavior. This didn't pop up in the release code but optimisations lead to (I assume) freeing of memory that shouldn't have been freed.

The rewrite of the deserializer avoids MaybeUninit altogether in favour of Option's at the cost of some performance, but the code is free of unsafe and sound.

I'll release this as 0.12.0 along with an entry in the advisory-db to push people to update form 0.11 (or earlier) and yank affected versions from crates.io

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants