bincode-org / bincode

A binary encoder / decoder implementation in Rust.
MIT License
2.69k stars 272 forks source link

Add a feature to disable extra debug info (#595) #596

Closed capatoles closed 2 years ago

capatoles commented 2 years ago

The issue is in bincode_derive crate, where we are adding extra debug info that are leaking enum names in the final binary (which imply less optimization with a bigger binary size, and internal structure leak). I think we should use a proper feature flag to disable this. This is linked to #595.

It seems there is only one leak in the code in this part of the code. I agree this debug line can be important, but should be optional for release build if required. A feature flag could be used to address that situation, and maybe it is useful for other cases like in https://github.com/bincode-org/bincode/issues/594.

This allows

VictorKoenders commented 2 years ago

So if I do:

fn main() {
    let bytes = bincode::encode_to_vec(Foo::Bar, bincode::config::standard()).map_err(|_| ()).unwrap();
    println!("{bytes:?}");
}

#[derive(bincode::Encode, bincode::Decode)]
enum Foo {
    Bar
}

And I run

cargo build --release
strip target/release/test_proj
strings target/release/test_proj

I cannot find any instance of Foo or Bar.

If you get rid of all .unwrap(), .expect() etc in your codebase, and map all errors into unit types, do you still notice the two issues you mention?

capatoles commented 2 years ago

The code in question is in the Decode part of the code, so you need to call a decode_* operation in your minimal example. For example:

fn main() {
    let bytes = bincode::encode_to_vec(Foo::Bar, bincode::config::standard()).map_err(|_| ()).unwrap();
    println!("{bytes:?}");
    let (_, _): (Foo, _) = bincode::decode_from_slice(&bytes, bincode::config::standard()).unwrap();
}

#[derive(bincode::Encode, bincode::Decode)]
enum Foo {
    Bar
}

Which gives (Foo in the result of the strings command):

$ cargo build --release
$ strip errors-and-strings
$ strings errors-and-strings | grep "Foo"
Foolibrary/alloc/src/raw_vec.rscapacity overflowlibrary/alloc/src/ffi/c_str.rs

You can also check that with the cargo-expand crate:

#![feature(prelude_import)]
#[prelude_import]
use std::prelude::rust_2021::*;

#[macro_use]
extern crate std;
fn main() {
    let bytes = bincode::encode_to_vec(Foo::Bar, bincode::config::standard())
        .map_err(|_| ())
        .unwrap();
    {
        ::std::io::_print(
            ::core::fmt::Arguments::new_v1(
                &["", "\n"],
                &[::core::fmt::ArgumentV1::new_debug(&bytes)],
            ),
        );
    };
    let (_, _): (Foo, _) = bincode::decode_from_slice(
            &bytes,
            bincode::config::standard(),
        )
        .unwrap();
}

enum Foo {
    Bar,
}

impl ::bincode::Encode for Foo {
    fn encode<__E: ::bincode::enc::Encoder>(
        &self,
        encoder: &mut __E,
    ) -> core::result::Result<(), ::bincode::error::EncodeError> {
        match self {
            Self::Bar => {
                <u32 as ::bincode::Encode>::encode(&(0u32), encoder)?;
                Ok(())
            }
        }
    }
}

impl ::bincode::Decode for Foo {
    fn decode<__D: ::bincode::de::Decoder>(
        decoder: &mut __D,
    ) -> core::result::Result<Self, ::bincode::error::DecodeError> {
        let variant_index = <u32 as ::bincode::Decode>::decode(decoder)?;
        match variant_index {
            0u32 => Ok(Self::Bar {}),
            variant => {
                Err(::bincode::error::DecodeError::UnexpectedVariant {
                    found: variant,
                    type_name: "Foo",
                    allowed: &::bincode::error::AllowedEnumVariants::Range {
                        min: 0,
                        max: 0,
                    },
                })
            }
        }
    }
}

impl<'__de> ::bincode::BorrowDecode<'__de> for Foo {
    fn borrow_decode<__D: ::bincode::de::BorrowDecoder<'__de>>(
        decoder: &mut __D,
    ) -> core::result::Result<Self, ::bincode::error::DecodeError> {
        let variant_index = <u32 as ::bincode::Decode>::decode(decoder)?;
        match variant_index {
            0u32 => Ok(Self::Bar {}),
            variant => {
                Err(::bincode::error::DecodeError::UnexpectedVariant {
                    found: variant,
                    type_name: "Foo",
                    allowed: &::bincode::error::AllowedEnumVariants::Range {
                        min: 0,
                        max: 0,
                    },
                })
            }
        }
    }
}

In the same idea, your debug file in ./target contains the string:

impl :: bincode :: Decode for Foo
{
    fn decode < __D : :: bincode :: de :: Decoder > (decoder : & mut __D) ->
    core :: result :: Result < Self, :: bincode :: error :: DecodeError >
    {
        let variant_index = < u32 as :: bincode :: Decode > :: decode(decoder)
        ? ; match variant_index
        {
            0u32 => Ok(Self :: Bar {}), variant =>
            Err(:: bincode :: error :: DecodeError :: UnexpectedVariant
            {
                found : variant, type_name : "Foo", allowed : & :: bincode ::
                error :: AllowedEnumVariants :: Range { min : 0, max : 0 }
            })
        }
    }
} impl < '__de > :: bincode :: BorrowDecode < '__de > for Foo
{
    fn borrow_decode < __D : :: bincode :: de :: BorrowDecoder < '__de > >
    (decoder : & mut __D) -> core :: result :: Result < Self, :: bincode ::
    error :: DecodeError >
    {
        let variant_index = < u32 as :: bincode :: Decode > :: decode(decoder)
        ? ; match variant_index
        {
            0u32 => Ok(Self :: Bar {}), variant =>
            Err(:: bincode :: error :: DecodeError :: UnexpectedVariant
            {
                found : variant, type_name : "Foo", allowed : & :: bincode ::
                error :: AllowedEnumVariants :: Range { min : 0, max : 0 }
            })
        }
    }
}
VictorKoenders commented 2 years ago

If you get rid of all .unwrap(), .expect() etc in your codebase

Can you try this?

capatoles commented 2 years ago

Thanks for the suggestion. I tried this but unfortunately the strings are still here, even if I get rid of the unwrap() and expect(). But if I activated some extra optimization (Like LTO optimization), the strings disappear. It makes me kind of sad that we depend on a compiler optimization that may or may not be done.

VictorKoenders commented 2 years ago

It makes me kind of sad that we depend on a compiler optimization that may or may not be done.

I think this is pretty common to deal with in your high-security domain? Surely enabling LTO is the very first thing you do in a new project.

Can I recommend using cargo-vendor to get a local copy of all the dependencies you're using? I'd imagine you want to audit every single one of them and apply minor patches to make all your dependencies suitable for your usecase. For bincode you might even want to consider removing all error variants you do not care about.

I'm going to close this and wait for the outcome of the discussion in #595