[SECURITY] Quick review of the crate

[jonasbb]

Hi, in dtolnay/request-for-implementation#17 (comment) you asked for some feedback.

use std::convert::TryInto;
use serde::{Deserialize, Serialize};

fn main() {
    // f1();
    f2();
}

/*
fn f1() {
    // error[E0277]: the trait bound `T: Serialize` is not satisfied
    #[serbia::serbia]
    #[derive(Debug, Serialize, Deserialize)]
    struct S<T> {
        arr_big: [T; 300],
    }

    let j = serde_json::json!({
        "arr_big": [""]
    });
    dbg!(serde_json::from_value::<S<String>>(j));
}
// */

// /*
fn f2() {
    // Crashes with
    // free(): double free detected in tcache 2
    #[serbia::serbia]
    #[derive(Debug, Serialize, Deserialize)]
    struct S {
        arr_big: [String; 300],
    }

    let mut i = 0;
    let arr_big: [String; 300] = std::iter::from_fn(|| {
        i += 1;
        Some(i.to_string())
    })
    .take(300)
    .collect::<Vec<_>>()
    .try_into()
    .unwrap();
    serde_json::from_str::<S>(&serde_json::to_string(&S { arr_big }).unwrap()).unwrap();

    let j = serde_json::json!({
        "arr_big": []
    });
    drop(dbg!(serde_json::from_value::<S>(j)));
    println!("Reached");
}
// */

f1() in this code just shows how generics currently do not work.

f2() however is a big security risk. Your array initialization is undefined behavior.
Given that the crate is used by at least one other crate you should consider yanking all affected crates. You can do that via the command line or via the web interface.
Adding a rustsec advisory might be a good option too.

[uint]

Hi. Thanks a lot for looking at this!

The memory thing is pretty embarrassing. I yanked the affected crates and fixed the issue, I think. I added your code as an integration test - hope that's okay!

I'll keep this issue around as a TODO for generics.

[jonasbb]

I added your code as an integration test - hope that's okay!
Sure. The code is a bit convoluted because without this line

serde_json::from_str::<S>(&serde_json::to_string(&S { arr_big }).unwrap()).unwrap();

I couldn't trigger any problems since the memory was 0 initialized. You should check the code with miri. It can warn you about undefined behavior and leaked memory. I am pretty sure the new code now leaks memory during errors, for example with "arr_big": ["abc"],.

[uint]

Does this basically illustrate your point about leaked memory?

use std::mem::MaybeUninit;

struct Foo(String);

impl Foo {
    fn new(s: impl ToString) -> Self {
        Foo(s.to_string())
    }
}

impl Drop for Foo {
    fn drop(&mut self) {
        println!("dropping {}", self.0);
    }
}

fn main() {
    {
        let mut foos: [MaybeUninit<Foo>; 5] = unsafe { MaybeUninit::uninit().assume_init() };
        
        for (i, e) in foos.iter_mut().enumerate().take(3) {
            *e = MaybeUninit::new(Foo::new(i));
        }
        // Now we have a partially initialized array, but elements are still in MaybeUninits.
    }
    
    // No "droppping ..." prints for the foos we did initialize! Oops.
    
    println!("Reached!");
}

I wonder if I could do some magic before returning the error to assume_init only the initialized part of the array and drop it.

I'm tempted to rewrite the whole thing without unsafe altogether, though.

[uint]

...or I could just learn from you, @jonasbb

[jonasbb]

Yes that shows the leaked memory problem. You are welcome to look at or copy from serde_with :) Unsafeless should also work by going from Vec to array.