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

empty libtest harness under cargo-careful fails valgrind #4

Closed
elichai opened this issue Oct 1, 2022 · 17 comments
Closed

empty libtest harness under cargo-careful fails valgrind #4

elichai opened this issue Oct 1, 2022 · 17 comments

Comments

@elichai
Copy link

elichai commented Oct 1, 2022

Steps to reproduce:

mkdir /tmp/memory-error && cd /tmp/memory-error
cargo init --lib
cargo +nightly careful test --no-run
valgrind --track-origins=yes target/debug/deps/memory_error-394e695bdd4a1e0f

Valgrind output:

==741342== Memcheck, a memory error detector
==741342== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al.
==741342== Using Valgrind-3.19.0 and LibVEX; rerun with -h for copyright info
==741342== Command: target/debug/deps/memory_error-394e695bdd4a1e0f
==741342== 
==741342== Conditional jump or move depends on uninitialised value(s)
==741342==    at 0x16673A: runtime<u8> (raw.rs:93)
==741342==    by 0x16673A: from_raw_parts<u8> (intrinsics.rs:2220)
==741342==    by 0x16673A: deref<u8, alloc::alloc::Global> (mod.rs:2533)
==741342==    by 0x16673A: index (string.rs:2185)
==741342==    by 0x16673A: eq (string.rs:2185)
==741342==    by 0x16673A: eq (lib.rs:626)
==741342==    by 0x16673A: eq<getopts::Name, getopts::Name> (lib.rs:933)
==741342==    by 0x16673A: {closure#0} (lib.rs:933)
==741342==    by 0x16673A: position<getopts::Opt, getopts::find_opt::{closure_env#0}> (macros.rs:295)
==741342==    by 0x16673A: getopts::find_opt (lib.rs:933)
==741342==    by 0x166084: getopts::Matches::opt_vals (lib.rs:797)
==741342==    by 0x16634A: getopts::Matches::opt_present (lib.rs:813)
==741342==    by 0x13C548: test::cli::parse_opts (cli.rs:207)
==741342==    by 0x15A0FC: test::test_main (lib.rs:95)
==741342==    by 0x15B0D1: test::test_main_static (lib.rs:131)
==741342==    by 0x123112: memory_error::main (lib.rs:1)
==741342==    by 0x12307A: core::ops::function::FnOnce::call_once (function.rs:251)
==741342==    by 0x12315D: std::sys_common::backtrace::__rust_begin_short_backtrace (backtrace.rs:122)
==741342==    by 0x122CE0: std::rt::lang_start::{{closure}} (rt.rs:166)
==741342==    by 0x18AA53: call_once<(), (dyn core::ops::function::Fn<(), Output=i32> + core::marker::Sync + core::panic::unwind_safe::RefUnwindSafe)> (function.rs:286)
==741342==    by 0x18AA53: do_call<&(dyn core::ops::function::Fn<(), Output=i32> + core::marker::Sync + core::panic::unwind_safe::RefUnwindSafe), i32> (panicking.rs:464)
==741342==    by 0x18AA53: try<i32, &(dyn core::ops::function::Fn<(), Output=i32> + core::marker::Sync + core::panic::unwind_safe::RefUnwindSafe)> (panicking.rs:428)
==741342==    by 0x18AA53: catch_unwind<&(dyn core::ops::function::Fn<(), Output=i32> + core::marker::Sync + core::panic::unwind_safe::RefUnwindSafe), i32> (panic.rs:137)
==741342==    by 0x18AA53: {closure#2} (rt.rs:148)
==741342==    by 0x18AA53: do_call<std::rt::lang_start_internal::{closure_env#2}, isize> (panicking.rs:464)
==741342==    by 0x18AA53: try<isize, std::rt::lang_start_internal::{closure_env#2}> (panicking.rs:428)
==741342==    by 0x18AA53: catch_unwind<std::rt::lang_start_internal::{closure_env#2}, isize> (panic.rs:137)
==741342==    by 0x18AA53: std::rt::lang_start_internal (rt.rs:148)
==741342==    by 0x122CB9: std::rt::lang_start (rt.rs:165)
==741342==  Uninitialised value was created by a stack allocation
==741342==    at 0x165FE7: getopts::Matches::opt_vals (lib.rs:796)
==741342== 
==741342== Conditional jump or move depends on uninitialised value(s)
==741342==    at 0x16680E: runtime<u8> (raw.rs:93)
==741342==    by 0x16680E: from_raw_parts<u8> (intrinsics.rs:2220)
==741342==    by 0x16680E: deref<u8, alloc::alloc::Global> (mod.rs:2533)
==741342==    by 0x16680E: index (string.rs:2185)
==741342==    by 0x16680E: eq (string.rs:2185)
==741342==    by 0x16680E: eq (lib.rs:626)
==741342==    by 0x16680E: eq<getopts::Name, getopts::Name> (lib.rs:940)
==741342==    by 0x16680E: {closure#1} (lib.rs:940)
==741342==    by 0x16680E: any<getopts::Opt, getopts::find_opt::{closure_env#1}> (macros.rs:242)
==741342==    by 0x16680E: getopts::find_opt (lib.rs:940)
==741342==    by 0x166084: getopts::Matches::opt_vals (lib.rs:797)
==741342==    by 0x16634A: getopts::Matches::opt_present (lib.rs:813)
==741342==    by 0x13C548: test::cli::parse_opts (cli.rs:207)
==741342==    by 0x15A0FC: test::test_main (lib.rs:95)
==741342==    by 0x15B0D1: test::test_main_static (lib.rs:131)
==741342==    by 0x123112: memory_error::main (lib.rs:1)
==741342==    by 0x12307A: core::ops::function::FnOnce::call_once (function.rs:251)
==741342==    by 0x12315D: std::sys_common::backtrace::__rust_begin_short_backtrace (backtrace.rs:122)
==741342==    by 0x122CE0: std::rt::lang_start::{{closure}} (rt.rs:166)
==741342==    by 0x18AA53: call_once<(), (dyn core::ops::function::Fn<(), Output=i32> + core::marker::Sync + core::panic::unwind_safe::RefUnwindSafe)> (function.rs:286)
==741342==    by 0x18AA53: do_call<&(dyn core::ops::function::Fn<(), Output=i32> + core::marker::Sync + core::panic::unwind_safe::RefUnwindSafe), i32> (panicking.rs:464)
==741342==    by 0x18AA53: try<i32, &(dyn core::ops::function::Fn<(), Output=i32> + core::marker::Sync + core::panic::unwind_safe::RefUnwindSafe)> (panicking.rs:428)
==741342==    by 0x18AA53: catch_unwind<&(dyn core::ops::function::Fn<(), Output=i32> + core::marker::Sync + core::panic::unwind_safe::RefUnwindSafe), i32> (panic.rs:137)
==741342==    by 0x18AA53: {closure#2} (rt.rs:148)
==741342==    by 0x18AA53: do_call<std::rt::lang_start_internal::{closure_env#2}, isize> (panicking.rs:464)
==741342==    by 0x18AA53: try<isize, std::rt::lang_start_internal::{closure_env#2}> (panicking.rs:428)
==741342==    by 0x18AA53: catch_unwind<std::rt::lang_start_internal::{closure_env#2}, isize> (panic.rs:137)
==741342==    by 0x18AA53: std::rt::lang_start_internal (rt.rs:148)
==741342==    by 0x122CB9: std::rt::lang_start (rt.rs:165)
==741342==  Uninitialised value was created by a stack allocation
==741342==    at 0x165FE7: getopts::Matches::opt_vals (lib.rs:796)
==741342== 
==741342== Conditional jump or move depends on uninitialised value(s)
==741342==    at 0x16673A: runtime<u8> (raw.rs:93)
==741342==    by 0x16673A: from_raw_parts<u8> (intrinsics.rs:2220)
==741342==    by 0x16673A: deref<u8, alloc::alloc::Global> (mod.rs:2533)
==741342==    by 0x16673A: index (string.rs:2185)
==741342==    by 0x16673A: eq (string.rs:2185)
==741342==    by 0x16673A: eq (lib.rs:626)
==741342==    by 0x16673A: eq<getopts::Name, getopts::Name> (lib.rs:933)
==741342==    by 0x16673A: {closure#0} (lib.rs:933)
==741342==    by 0x16673A: position<getopts::Opt, getopts::find_opt::{closure_env#0}> (macros.rs:295)
==741342==    by 0x16673A: getopts::find_opt (lib.rs:933)
==741342==    by 0x166084: getopts::Matches::opt_vals (lib.rs:797)
==741342==    by 0x166643: getopts::Matches::opt_str (lib.rs:804)
==741342==    by 0x13C764: get_allow_unstable (cli.rs:471)
==741342==    by 0x13C764: parse_opts_impl (cli.rs:252)
==741342==    by 0x13C764: test::cli::parse_opts (cli.rs:214)
==741342==    by 0x15A0FC: test::test_main (lib.rs:95)
==741342==    by 0x15B0D1: test::test_main_static (lib.rs:131)
==741342==    by 0x123112: memory_error::main (lib.rs:1)
==741342==    by 0x12307A: core::ops::function::FnOnce::call_once (function.rs:251)
==741342==    by 0x12315D: std::sys_common::backtrace::__rust_begin_short_backtrace (backtrace.rs:122)
==741342==    by 0x122CE0: std::rt::lang_start::{{closure}} (rt.rs:166)
==741342==    by 0x18AA53: call_once<(), (dyn core::ops::function::Fn<(), Output=i32> + core::marker::Sync + core::panic::unwind_safe::RefUnwindSafe)> (function.rs:286)
==741342==    by 0x18AA53: do_call<&(dyn core::ops::function::Fn<(), Output=i32> + core::marker::Sync + core::panic::unwind_safe::RefUnwindSafe), i32> (panicking.rs:464)
==741342==    by 0x18AA53: try<i32, &(dyn core::ops::function::Fn<(), Output=i32> + core::marker::Sync + core::panic::unwind_safe::RefUnwindSafe)> (panicking.rs:428)
==741342==    by 0x18AA53: catch_unwind<&(dyn core::ops::function::Fn<(), Output=i32> + core::marker::Sync + core::panic::unwind_safe::RefUnwindSafe), i32> (panic.rs:137)
==741342==    by 0x18AA53: {closure#2} (rt.rs:148)
==741342==    by 0x18AA53: do_call<std::rt::lang_start_internal::{closure_env#2}, isize> (panicking.rs:464)
==741342==    by 0x18AA53: try<isize, std::rt::lang_start_internal::{closure_env#2}> (panicking.rs:428)
==741342==    by 0x18AA53: catch_unwind<std::rt::lang_start_internal::{closure_env#2}, isize> (panic.rs:137)
==741342==    by 0x18AA53: std::rt::lang_start_internal (rt.rs:148)
==741342==    by 0x122CB9: std::rt::lang_start (rt.rs:165)
==741342==  Uninitialised value was created by a stack allocation
==741342==    at 0x165FE7: getopts::Matches::opt_vals (lib.rs:796)
==741342== 

running 1 test
test tests::it_works ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.12s

==741342== 
==741342== HEAP SUMMARY:
==741342==     in use at exit: 0 bytes in 0 blocks
==741342==   total heap usage: 479 allocs, 479 frees, 53,506 bytes allocated
==741342== 
==741342== All heap blocks were freed -- no leaks are possible
==741342== 
==741342== For lists of detected and suppressed errors, rerun with: -s
==741342== ERROR SUMMARY: 3 errors from 3 contexts (suppressed: 0 from 0)

I've tried to debug this with gdb and adding prints, but it disappears if you look at it too closely so gave up after a bit of trying

@RalfJung
Copy link
Owner

RalfJung commented Oct 1, 2022

Which binary are you running here? I am confused what this is a bug in. But it seems to me this is running the empty libtest harness and pointing at something in there? So it would be a bug in libtest, getopts, or valgrind, but not cargo-careful?

That libtest harness is extensively tested by Miri though.

@RalfJung RalfJung changed the title Generated binary fails valgrind empty libtest harness undder cargo-careful fails valgrind Oct 1, 2022
@RalfJung RalfJung changed the title empty libtest harness undder cargo-careful fails valgrind empty libtest harness under cargo-careful fails valgrind Oct 1, 2022
@elichai
Copy link
Author

elichai commented Oct 1, 2022

Which binary are you running here? I am confused what this is a bug in. But it seems to me this is running the empty libtest harness and pointing at something in there?

yeah, it fails on libtest even before getting to the tests.

So it would be a bug in libtest, getopts, or valgrind, but not cargo-careful?

I agree, but it only happens under the cargo-careful configuration, and not with a regular cargo test binary, so I decided to open it here because cargo-careful is the easiest way to reproduce the problem, but I can open an issue in rust-lang/rust instead.

@elichai
Copy link
Author

elichai commented Oct 1, 2022

Note that getopts+unicode-width(the only dep getopts has) both have 0 unsafe in them.

@RalfJung
Copy link
Owner

RalfJung commented Oct 1, 2022

If you use regular cargo and set

RUSTFLAGS="-Zstrict-init-checks -Zextra-const-ub-checks -Cdebug-assertions=on"

does that trigger the problem?

@elichai
Copy link
Author

elichai commented Oct 1, 2022

I assume you meant RUSTFLAGS="-Zstrict-init-checks -Zextra-const-ub-checks -Cdebug-assertions=on" cargo +nightly test -Z build-std --target=x86_64-unknown-linux-gnu
And it doesn't seem to trigger the problem

@RalfJung
Copy link
Owner

RalfJung commented Oct 1, 2022

I don't know what exactly build-std does, it might not apply the flags to the standard library.

So looks like this has to do with compiling the standard library with debug assertions?

@elichai
Copy link
Author

elichai commented Oct 1, 2022

So looks like this has to do with compiling the standard library with debug assertions?

As far as I can tell, yes

@RalfJung
Copy link
Owner

RalfJung commented Oct 1, 2022

Assuming the line numbers are correct, it points at this debug assertion

https://github.com/rust-lang/rust/blob/075084f772abbc53263fc946c047a01e0dd65f80/library/core/src/slice/raw.rs#L93-L96

None of this can really be uninit though, given that Miri has been all over that code, so my assumption is that LLVM did something odd to the assembly that confuses valgrind, or some such thing. That's way outside of what I can debug, sorry.

@tavianator
Copy link

So we have

000000000006a380 <_ZN7getopts7Matches8opt_vals17hbf0997a509768c43E>:
   6a380:       41 57                   push   %r15
   6a382:       41 56                   push   %r14
   6a384:       41 54                   push   %r12
   6a386:       53                      push   %rbx
   6a387:       48 83 ec 68             sub    $0x68,%rsp
   6a38b:       48 89 54 24 18          mov    %rdx,0x18(%rsp)
   6a390:       48 89 4c 24 20          mov    %rcx,0x20(%rsp)
   6a395:       4c 8b 3e                mov    (%rsi),%r15
   6a398:       41 f6 c7 07             test   $0x7,%r15b
   6a39c:       0f 85 e6 00 00 00       jne    6a488 <_ZN7getopts7Matches8opt_vals17hbf0997a509768c43E+0x108>
   6a3a2:       49 89 d0                mov    %rdx,%r8
   6a3a5:       48 89 f3                mov    %rsi,%rbx
   6a3a8:       4c 8b 66 10             mov    0x10(%rsi),%r12
   6a3ac:       ba 38 00 00 00          mov    $0x38,%edx
   6a3b1:       4c 89 e0                mov    %r12,%rax
   6a3b4:       48 f7 e2                mul    %rdx
   6a3b7:       0f 80 cb 00 00 00       jo     6a488 <_ZN7getopts7Matches8opt_vals17hbf0997a509768c43E+0x108>
   6a3bd:       48 85 c0                test   %rax,%rax
   6a3c0:       0f 88 c2 00 00 00       js     6a488 <_ZN7getopts7Matches8opt_vals17hbf0997a509768c43E+0x108>
   6a3c6:       49 89 fe                mov    %rdi,%r14
   6a3c9:       48 83 f9 01             cmp    $0x1,%rcx
   6a3cd:       75 12                   jne    6a3e1 <_ZN7getopts7Matches8opt_vals17hbf0997a509768c43E+0x61>
   6a3cf:       41 0f b6 00             movzbl (%r8),%eax
   6a3d3:       89 44 24 08             mov    %eax,0x8(%rsp)
   6a3d7:       48 c7 04 24 00 00 00    movq   $0x0,(%rsp)
   6a3de:       00 
   6a3df:       eb 0e                   jmp    6a3ef <_ZN7getopts7Matches8opt_vals17hbf0997a509768c43E+0x6f>
   6a3e1:       48 89 e7                mov    %rsp,%rdi
   6a3e4:       4c 89 c6                mov    %r8,%rsi
   6a3e7:       48 89 ca                mov    %rcx,%rdx
   6a3ea:       e8 71 31 00 00          call   6d560 <_ZN5alloc5slice64_$LT$impl$u20$alloc..borrow..ToOwned$u20$for$u20$$u5b$T$u5d$$GT$8to_owned17he2b6ed39784f2498E>
   6a3ef:       48 89 e2                mov    %rsp,%rdx
   6a3f2:       4c 89 ff                mov    %r15,%rdi
   6a3f5:       4c 89 e6                mov    %r12,%rsi
   6a3f8:       ff 15 b2 87 0b 00       call   *0xb87b2(%rip)        # 122bb0 <_GLOBAL_OFFSET_TABLE_+0x8f8>
...

The call is to

000000000006a790 <_ZN7getopts8find_opt17ha49ce0943149df1eE>:
   6a790:       55                      push   %rbp
   6a791:       41 57                   push   %r15
   6a793:       41 56                   push   %r14
   6a795:       41 55                   push   %r13
   6a797:       41 54                   push   %r12
   6a799:       53                      push   %rbx
   6a79a:       48 83 ec 28             sub    $0x28,%rsp
   6a79e:       48 85 f6                test   %rsi,%rsi
   6a7a1:       0f 84 a6 00 00 00       je     6a84d <_ZN7getopts8find_opt17ha49ce0943149df1eE+0xbd>
   6a7a7:       49 89 fe                mov    %rdi,%r14
   6a7aa:       4c 6b ce 38             imul   $0x38,%rsi,%r9
   6a7ae:       4c 8b 2a                mov    (%rdx),%r13
   6a7b1:       4c 8b 42 10             mov    0x10(%rdx),%r8
   6a7b5:       44 8b 52 08             mov    0x8(%rdx),%r10d
   6a7b9:       4d 85 c0                test   %r8,%r8
   6a7bc:       4c 89 4c 24 10          mov    %r9,0x10(%rsp)
   6a7c1:       44 89 54 24 0c          mov    %r10d,0xc(%rsp)
   6a7c6:       4c 89 44 24 20          mov    %r8,0x20(%rsp)
   6a7cb:       0f 88 83 00 00 00       js     6a854 <_ZN7getopts8find_opt17ha49ce0943149df1eE+0xc4>

The last conditional jump is the one flagged by Valgrind. Relevant instructions are:

   6a7b1:       4c 8b 42 10             mov    0x10(%rdx),%r8
...
   6a7b9:       4d 85 c0                test   %r8,%r8
...
   6a7cb:       0f 88 83 00 00 00       js     6a854 <_ZN7getopts8find_opt17ha49ce0943149df1eE+0xc4>

So Valgrind thinks that 0x10(%rdx) is uninitialized. The caller opt_vals() passes its %rsp as %rdx:

...
   6a387:       48 83 ec 68             sub    $0x68,%rsp
   6a38b:       48 89 54 24 18          mov    %rdx,0x18(%rsp)
   6a390:       48 89 4c 24 20          mov    %rcx,0x20(%rsp)
...
   6a3d3:       89 44 24 08             mov    %eax,0x8(%rsp)
   6a3d7:       48 c7 04 24 00 00 00    movq   $0x0,(%rsp)
   6a3de:       00 
   6a3df:       eb 0e                   jmp    6a3ef <_ZN7getopts7Matches8opt_vals17hbf0997a509768c43E+0x6f>
...
   6a3ef:       48 89 e2                mov    %rsp,%rdx
...
   6a3f8:       ff 15 b2 87 0b 00       call   *0xb87b2(%rip)        # 122bb0 <_GLOBAL_OFFSET_TABLE_+0x8f8>

And it looks like Valgrind is right. opt_vals() never stores anything to 0x10(%rsp).

The value at %rsp is of type enum Name:

enum Name {
    /// A string representing the long name of an option.
    /// For example: "help"
    Long(String),
    /// A char representing the short name of an option.
    /// For example: 'h'
    Short(char),
}

and it's the Short variant. The conditional jump in find_opt() is checking the sign bit at offset 16 bytes into the enum Name instance. I assume that's padding in Name::Short, but I'm not sure what the layout of Name is exactly.

It does seem weird that find_opt() is checking the 16th byte of a Name before checking which variant it is, but it is not necessarily an incorrect compilation. Maybe both sides of the conditional branch end up checking which variant it is before using the result. I think the code is inlined from the derived PartialEq impl for Name:

fn find_opt(opts: &[Opt], nm: &Name) -> Option<usize> {
    // Search main options.
    let pos = opts.iter().position(|opt| &opt.name == nm);
...

@tavianator
Copy link

Here's a reduction that doesn't depend on the test harness:

main.rs
use std::hint::black_box;

/// Name of an option. Either a string or a single char.
#[derive(Clone, Debug, PartialEq, Eq)]
enum Name {
    /// A string representing the long name of an option.
    /// For example: "help"
    Long(String),
    /// A char representing the short name of an option.
    /// For example: 'h'
    Short(char),
}

use self::Name::*;

impl Name {
    fn from_str(nm: &str) -> Name {
        if nm.len() == 1 {
            Short(nm.as_bytes()[0] as char)
        } else {
            Long(nm.to_string())
        }
    }
}

/// Describes whether an option has an argument.
#[derive(Clone, Debug, Copy, PartialEq, Eq)]
pub enum HasArg {
    /// The option requires an argument.
    Yes,
    /// The option takes no argument.
    No,
    /// The option argument is optional.
    Maybe,
}

/// Describes how often an option may occur.
#[derive(Clone, Debug, Copy, PartialEq, Eq)]
pub enum Occur {
    /// The option occurs once.
    Req,
    /// The option occurs at most once.
    Optional,
    /// The option occurs zero or more times.
    Multi,
}

/// A description of a possible option.
#[derive(Clone, Debug, PartialEq, Eq)]
struct Opt {
    /// Name of the option
    name: Name,
    /// Whether it has an argument
    hasarg: HasArg,
    /// How often it can occur
    occur: Occur,
    /// Which options it aliases
    aliases: Vec<Opt>,
}

/// Describes whether an option is given at all or has a value.
#[derive(Clone, Debug, PartialEq, Eq)]
enum Optval {
    Val(String),
    Given,
}

fn find_opt(opts: &[Opt], nm: &Name) -> Option<usize> {
    // Search main options.
    let pos = opts.iter().position(|opt| &opt.name == nm);
    if pos.is_some() {
        return pos;
    }

    // Search in aliases.
    for candidate in opts.iter() {
        if candidate.aliases.iter().any(|opt| &opt.name == nm) {
            return opts.iter().position(|opt| opt.name == candidate.name);
        }
    }

    None
}

/// The result of checking command line arguments. Contains a vector
/// of matches and a vector of free strings.
#[derive(Clone, Debug, PartialEq, Eq)]
pub struct Matches {
    /// Options that matched
    opts: Vec<Opt>,
    /// Values of the Options that matched and their positions
    vals: Vec<Vec<(usize, Optval)>>,
    /// Free string fragments
    pub free: Vec<String>,
}

impl Matches {
    fn opt_vals(&self, nm: &str) -> Vec<(usize, Optval)> {
        match find_opt(&self.opts, &Name::from_str(nm)) {
            Some(id) => self.vals[id].clone(),
            None => panic!("No option '{}' defined", nm),
        }
    }
}

fn main() {
    let matches = Matches {
        opts: vec![
            Opt {
                name: Name::from_str("h"),
                hasarg: HasArg::No,
                occur: Occur::Optional,
                aliases: vec![],
            },
        ],
        vals: vec![
            vec![(0, Optval::Given)],
        ],
        free: vec![],
    };

    let vals = black_box(matches).opt_vals(black_box("h"));
    println!("{vals:?}");
}
tavianator@graphene $ cargo +nightly careful run --release
Preparing a careful sysroot (target: x86_64-unknown-linux-gnu)... done
   Compiling careful-bug-4 v0.1.0 (/home/tavianator/code/careful-bug-4)
warning: variant `Val` is never constructed
  --> src/main.rs:64:5
   |
63 | enum Optval {
   |      ------ variant in this enum
64 |     Val(String),
   |     ^^^
   |
   = note: `Optval` has derived impls for the traits `Debug` and `Clone`, but these are intentionally ignored during dead code analysis
   = note: `#[warn(dead_code)]` on by default

warning: `careful-bug-4` (bin "careful-bug-4") generated 1 warning
    Finished release [optimized] target(s) in 0.36s
     Running `target/release/careful-bug-4`
[(0, Given)]
tavianator@graphene $ valgrind target/release/careful-bug-4
==44884== Memcheck, a memory error detector
==44884== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al.
==44884== Using Valgrind-3.19.0 and LibVEX; rerun with -h for copyright info
==44884== Command: target/release/careful-bug-4
==44884== 
==44884== Conditional jump or move depends on uninitialised value(s)
==44884==    at 0x1158A2: careful_bug_4::main (in /home/tavianator/code/careful-bug-4/target/release/careful-bug-4)
==44884==    by 0x114522: std::sys_common::backtrace::__rust_begin_short_backtrace (in /home/tavianator/code/careful-bug-4/target/release/careful-bug-4)
==44884==    by 0x114F18: std::rt::lang_start::{{closure}} (in /home/tavianator/code/careful-bug-4/target/release/careful-bug-4)
==44884==    by 0x127D03: std::rt::lang_start_internal (in /home/tavianator/code/careful-bug-4/target/release/careful-bug-4)
==44884==    by 0x115EE7: main (in /home/tavianator/code/careful-bug-4/target/release/careful-bug-4)
==44884== 
[(0, Given)]
==44884== 
==44884== HEAP SUMMARY:
==44884==     in use at exit: 0 bytes in 0 blocks
==44884==   total heap usage: 15 allocs, 15 frees, 3,325 bytes allocated
==44884== 
==44884== All heap blocks were freed -- no leaks are possible
==44884== 
==44884== Use --track-origins=yes to see where uninitialised values come from
==44884== For lists of detected and suppressed errors, rerun with: -s
==44884== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

@RalfJung
Copy link
Owner

Does this reproduce without careful, ie

RUSTFLAGS="-Zstrict-init-checks -Zextra-const-ub-checks -Cdebug-assertions=on" cargo +nightly run --release

@tavianator
Copy link

Nope, looks like it needs the standard library to be built with those flags too. Ultimately I think this comes down to impl Index<RangeFull> for String calling str::from_utf8_unchecked which calls mem::transmute which I assume has some extra checks added by those flags.

@RalfJung
Copy link
Owner

According to the valgrind location, it's this check in from_raw_parts.

Probably copying enough things from the stdlib to the testcase can reproduce it without the custom stdlib -- not sure.

@elichai
Copy link
Author

elichai commented Oct 11, 2022

Reproduced this without cargo-careful:

main.rs
#![feature(core_intrinsics, ptr_internals, strict_provenance, pointer_is_aligned)]
use core::intrinsics::discriminant_value;
use core::{alloc::Layout, ffi::c_int, hint::black_box, mem, ptr};
use std::{hint::unreachable_unchecked, ptr::NonNull};
pub struct Vec<T> {
  ptr: NonNull<T>,
  cap: usize,
  len: usize,
}

impl<T> Vec<T> {
  pub fn with_cap(cap: usize) -> Self {
      let layout = Layout::array::<T>(cap).unwrap();
      if usize::BITS < 64 && layout.size() > isize::MAX as usize {
          unimplemented!();
      }
      let ptr = unsafe { std::alloc::alloc(layout) };
      Self {
          ptr: NonNull::new(ptr).unwrap().cast(),
          cap,
          len: 0,
      }
  }
  pub fn from_slice(s: &[T]) -> Self {
      let mut vec = Self::with_cap(s.len());
      unsafe { ptr::copy_nonoverlapping(s.as_ptr(), vec.ptr.as_mut(), s.len()) };
      vec.len = s.len();
      vec
  }
  pub fn as_slice(&self) -> &[T] {
      let data = self.as_ptr();
      let len = self.len;
      if is_aligned_and_not_null(data)
          && crate::mem::size_of::<T>().saturating_mul(len) <= isize::MAX as usize
      {
          panic!();
      }
      unsafe { &*ptr::slice_from_raw_parts(data, len) }
  }
  pub fn as_ptr(&self) -> *const T {
      self.ptr.as_ptr()
  }
}

impl<T> PartialEq for Vec<T> {
  fn eq(&self, other: &Self) -> bool {
      let s = self.as_slice();
      let o = other.as_slice();

      if s.len() != o.len() {
          return false;
      }
      extern "C" {
          fn memcmp(s1: *const u8, s2: *const u8, n: usize) -> c_int;
      }
      unsafe {
          let size = mem::size_of_val(self);
          memcmp(s.as_ptr() as *const u8, o.as_ptr() as *const u8, size) == 0
      }
  }
}

struct String {
  vec: Vec<u8>,
}

impl String {
  fn from_str(s: &str) -> String {
      let bytes = s.as_bytes();
      String {
          vec: Vec::from_slice(bytes),
      }
  }
}

impl PartialEq for String {
  fn eq(&self, other: &String) -> bool {
      self.vec == other.vec
  }
}

enum Name {
  Long(String),
  Short(char),
}

impl PartialEq for Name {
  fn eq(&self, other: &Name) -> bool {
      let self_tag = discriminant_value(self);
      let other_tag = discriminant_value(other);
      self_tag == other_tag
          && match (self, other) {
              (Name::Long(self_0), Name::Long(other_0)) => self_0 == other_0,
              (Name::Short(self_0), Name::Short(other_0)) => self_0 == other_0,
              _ => unsafe { unreachable_unchecked() },
          }
  }
}

impl Name {
  fn from_str(nm: &str) -> Name {
      if nm.len() == 1 {
          Name::Short(nm.as_bytes()[0] as char)
      } else {
          Name::Long(String::from_str(nm))
      }
  }
}

struct Opt {
  name: Name,
}

#[inline(never)]
fn find_opt(opts: &[Opt], nm: &Name) -> Option<usize> {
  // Search main options.
  let mut pos = None;
  for (i, opt) in opts.iter().enumerate() {
      if &opt.name == nm {
          pos = Some(i);
          break;
      }
  }
  pos
}

fn main() {
  let opts = &[Opt {
      name: Name::from_str("h"),
  }][..];
  let nm = "h";
  let name = Name::from_str(nm);

  black_box(find_opt(opts, &name));
}
pub(crate) fn is_aligned_and_not_null<T>(ptr: *const T) -> bool {
  !ptr.is_null() && ptr.is_aligned()
}

(I'll work on minimizing this more)

This fails with:

cargo +nightly run --release

valgrind output:

==386741== Conditional jump or move depends on uninitialised value(s)
==386741==    at 0x10FC0F: saturating_mul (main.rs:34)
==386741==    by 0x10FC0F: as_slice<u8> (main.rs:34)
==386741==    by 0x10FC0F: eq<u8> (main.rs:48)
==386741==    by 0x10FC0F: eq (main.rs:78)
==386741==    by 0x10FC0F: eq<wat::String, wat::String> (main.rs:93)
==386741==    by 0x10FC0F: eq (main.rs:93)
==386741==    by 0x10FC0F: eq<wat::Name, wat::Name> (main.rs:119)
==386741==    by 0x10FC0F: wat::find_opt (main.rs:119)
==386741==    by 0x10FCE4: wat::main (main.rs:134)
==386741==    by 0x10FD42: call_once<fn(), ()> (function.rs:251)
==386741==    by 0x10FD42: std::sys_common::backtrace::__rust_begin_short_backtrace (backtrace.rs:122)
==386741==    by 0x10FBC8: std::rt::lang_start::{{closure}} (rt.rs:166)
==386741==    by 0x1223CE: call_once<(), (dyn core::ops::function::Fn<(), Output=i32> + core::marker::Sync + core::panic::unwind_safe::RefUnwindSafe)> (function.rs:286)
==386741==    by 0x1223CE: do_call<&(dyn core::ops::function::Fn<(), Output=i32> + core::marker::Sync + core::panic::unwind_safe::RefUnwindSafe), i32> (panicking.rs:464)
==386741==    by 0x1223CE: try<i32, &(dyn core::ops::function::Fn<(), Output=i32> + core::marker::Sync + core::panic::unwind_safe::RefUnwindSafe)> (panicking.rs:428)
==386741==    by 0x1223CE: catch_unwind<&(dyn core::ops::function::Fn<(), Output=i32> + core::marker::Sync + core::panic::unwind_safe::RefUnwindSafe), i32> (panic.rs:137)
==386741==    by 0x1223CE: {closure#2} (rt.rs:148)
==386741==    by 0x1223CE: do_call<std::rt::lang_start_internal::{closure_env#2}, isize> (panicking.rs:464)
==386741==    by 0x1223CE: try<isize, std::rt::lang_start_internal::{closure_env#2}> (panicking.rs:428)
==386741==    by 0x1223CE: catch_unwind<std::rt::lang_start_internal::{closure_env#2}, isize> (panic.rs:137)
==386741==    by 0x1223CE: std::rt::lang_start_internal (rt.rs:148)
==386741==    by 0x10FD2E: main (in /tmp/wat/target/release/wat)
==386741==  Uninitialised value was created by a stack allocation
==386741==    at 0x10FCB0: wat::main (main.rs:127)

@elichai
Copy link
Author

elichai commented Oct 11, 2022

I think the exact lines of valgrind are wrong, this might be a miscompilation thing

@RalfJung
Copy link
Owner

Miri is fine with that code... so yeah either a miscompilation or a valgrind false positive.

Sounds like this should be turned into a rustc bug report then?

@elichai
Copy link
Author

elichai commented Oct 12, 2022

Sounds like this should be turned into a rustc bug report then?

Yes, I edited rust-lang/rust#102712 to reflect the conversation here, so I guess this can be closed

@elichai elichai closed this as completed Oct 12, 2022
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

No branches or pull requests

3 participants