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

Array reads leak resources owned by successfully constructed elements if a subsequent element read fails #264

Closed
wildbook opened this issue Jun 10, 2022 · 2 comments
Assignees
Labels
bug Something isn't working

Comments

@wildbook
Copy link

My bad for not initially catching this when looking over the fix in #254. I just looked over the code again while showing it to a friend and realized that Drop isn't called for successfully read elements if a subsequent element read fails.

It's not UB to not call Drop (or std::mem::forget would need to be unsafe), so this isn't as critical as #263, but I imagine it's still desirable to call it and free potentially allocated resources when possible.


Minimal example:

use deku::{DekuContainerRead, DekuRead};

fn main() {
    println!("{:?}", Foo::from_bytes((&[1, 2], 0)));
}

#[derive(DekuRead, Debug)]
struct Foo([Bar; 3]);

#[derive(Debug)]
struct Bar(u8);

impl<'a> DekuRead<'a> for Bar {
    fn read(input: &'a deku::bitvec::BitSlice<deku::bitvec::Msb0, u8>, ctx: ())
        -> Result<(&'a deku::bitvec::BitSlice<deku::bitvec::Msb0, u8>, Self), deku::DekuError>
    where
        Self: Sized,
    {
        let (input, value) = u8::read(input, ctx)?;
        // This could allocate memory or otherwise acquire resources that should be freed
        println!("+1");
        Ok((input, Bar(value)))
    }
}

impl Drop for Bar {
    fn drop(&mut self) {
        // If N elements are read and the N+1th element fails, this is never called
        println!("-1: {:x?}", self);
    }
}
+1
+1
Err(Incomplete(NeedSize { bits: 8 }))

I'm guessing the most common case this affects is something like this:

fn main() {
    // There's not enough input data so the second `Bar::read` will fail, but only after
    // allocating memory during the first `Bar::read` call. Since existing elements aren't
    // dropped on failure we're leaking the first `Bar` instance's allocated resources, in this
    // case the Vec's backing memory.
    println!("{:?}", Foo::from_bytes((&[1], 0)));
}

#[derive(DekuRead, Debug)]
struct Foo([Bar; 2]);

#[derive(DekuRead, Debug)]
struct Bar {
    #[deku(count = "1")]
    vec: Vec<u8>,
    // ...
}
@sharksforarms sharksforarms added the bug Something isn't working label Jul 4, 2022
wcampbell0x2a added a commit that referenced this issue Jan 20, 2023
From the example in #264, the following output is seen:

+1
+1
-1: Bar(1)
-1: Bar(2)
Err(Incomplete(NeedSize { bits: 8 }))
@wcampbell0x2a wcampbell0x2a self-assigned this Jan 20, 2023
wcampbell0x2a added a commit that referenced this issue Jan 20, 2023
From the example in #264, the following output is seen:

+1
+1
-1: Bar(1)
-1: Bar(2)
Err(Incomplete(NeedSize { bits: 8 }))
sharksforarms pushed a commit that referenced this issue Feb 2, 2023
From the example in #264, the following output is seen:

+1
+1
-1: Bar(1)
-1: Bar(2)
Err(Incomplete(NeedSize { bits: 8 }))
@wcampbell0x2a
Copy link
Collaborator

@wildbook can you confirm the fixes merged with #317?

@wildbook
Copy link
Author

I can confirm that fix looks proper to me. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants