Skip to content

Memory sanitizer : false positive regression in nightly-2023-01-18 #107149

@catenacyber

Description

@catenacyber

Code

This comes from oss-fuzz cf https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=55286&q=label%3AProj-suricata

This is with RUSTFLAGS='--cfg fuzzing -Zsanitizer=memory -Cdebuginfo=1 -Cforce-frame-pointers -Zsanitizer-memory-track-origins'

It happens with C code calling a rust function which returns std::ptr::null_mut(), and then C code checks the pointer for NULL

Basically, it reports use of uninitialized value at
https://github.com/OISF/suricata/blob/a24d7dc45c818054f97448ce42ca9ba270b3b8e4/src/detect-dce-iface.c#L151

    void *did = rs_dcerpc_iface_parse(arg);
    if (did == NULL) {

And rs_dcerpc_iface_parse is returning std::ptr::null_mut()
cf https://github.com/OISF/suricata/blob/a24d7dc45c818054f97448ce42ca9ba270b3b8e4/rust/src/dcerpc/detect.rs#L243

I expected to see this happen: no report from Memory sanitizer

Instead, this happened:

==13==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0xe25448 in DetectDceIfaceSetup /src/suricata/src/detect-dce-iface.c:151:9

Version it worked on

It most recently worked on: nightly-2023-01-17

Version with regression

rustc --version --verbose:

Sorry it is nightly-2023-01-18

rustc --version --verbose
rustc 1.68.0-nightly (3984bc583 2023-01-17)
binary: rustc
commit-hash: 3984bc5833db8bfb0acc522c9775383e4171f3de
commit-date: 2023-01-17
host: x86_64-unknown-linux-gnu
release: 1.68.0-nightly
LLVM version: 15.0.6

Activity

added
I-prioritizeIssue: Indicates that prioritization has been requested for this issue.
on Jan 21, 2023
catenacyber

catenacyber commented on Jan 21, 2023

@catenacyber
Author

And it worked on rustup default nightly-2023-01-17

rustc 1.68.0-nightly (4781233a7 2023-01-16)
binary: rustc
commit-hash: 4781233a77e879e49cb5ce3c98d2abba6a6ade7a
commit-date: 2023-01-16
host: x86_64-unknown-linux-gnu
release: 1.68.0-nightly
LLVM version: 15.0.6
``
tmiasko

tmiasko commented on Jan 21, 2023

@tmiasko
Contributor

That looks like noundef mismatch between C and Rust on return value due to #106294. Building Rust with -Cllvm-args=-msan-eager-checks=0 might help (and -fnosanitize-memory-param-retval in clang), but ultimately this probably requires clang with llvm/llvm-project@166c8cc.

added a commit that references this issue on Jan 21, 2023
catenacyber

catenacyber commented on Jan 21, 2023

@catenacyber
Author

Thank you very much @tmiasko
-Cllvm-args=-msan-eager-checks=0 does the trick cf google/oss-fuzz#9478

C is compiled with

clang version 15.0.0 (https://github.com/llvm/llvm-project.git bf7f8d6fa6f460bf0a16ffec319cd71592216bf4)
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: /usr/local/bin
added a commit that references this issue on Jan 21, 2023
elichai

elichai commented on Jan 22, 2023

@elichai
Contributor

We encountered this in: rust-bitcoin/rust-secp256k1#573

And was just about to open an issue with this Minimal Reproducible Example: (a few lines of code) https://github.com/elichai/msan_c_rust_bug

but it sounds like this is a bug coming from mismatching llvm definitions?

added a commit that references this issue on Jan 22, 2023
added a commit that references this issue on Jan 22, 2023

1 remaining item

added a commit that references this issue on Jan 23, 2023
0e689c7
apiraino

apiraino commented on Jan 25, 2023

@apiraino
Contributor

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-high

added
P-highHigh priority
and removed
I-prioritizeIssue: Indicates that prioritization has been requested for this issue.
on Jan 25, 2023
added
T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.
on Jan 25, 2023
RalfJung

RalfJung commented on Jan 27, 2023

@RalfJung
Member

Having noundef on the definition but not the declaration (or vice versa) should be totally fine if the return value is indeed always noundef. So I am surprised that the sanitizer complains here, how would the presence of the attribute (without a change in behavior) make any difference?

catenacyber

catenacyber commented on Mar 31, 2023

@catenacyber
Author

Looks like there was another regression between rustc 1.70.0-nightly (8be3c2bda 2023-03-24) and rustc 1.70.0-nightly (0c61c7a97 2023-03-25)
I now see

/usr/bin/ld: ../rust/target/x86_64-unknown-linux-gnu/release/libsuricata_rust.a(suricata-47b7dd5053e4a0cf.suricata.f4469a01-cgu.1.rcgu.o): in function `<(FnA,FnB,FnC) as nom::sequence::Tuple<Input,(A,B,C),Error>>::parse':
 /rust/registry/src/index.crates.io-6f17d22bba15001f/nom-7.1.3/src/sequence/mod.rs:227: undefined reference to `__msan_set_alloca_origin_with_descr'

Is this the same issue or another one ?

tmiasko

tmiasko commented on Mar 31, 2023

@tmiasko
Contributor

Mismatch in LLVM version between clang and rustc? Rust was upgraded to LLVM 16 in #109474. Also, clang 16 enabled sanitize-memory-param-retval, which matches rustc defaults now.

catenacyber

catenacyber commented on Apr 3, 2023

@catenacyber
Author

Thanks @tmiasko :-)

added a commit that references this issue on Apr 4, 2023
pnkfelix

pnkfelix commented on Jun 30, 2023

@pnkfelix
Contributor

Having noundef on the definition but not the declaration (or vice versa) should be totally fine if the return value is indeed always noundef. So I am surprised that the sanitizer complains here, how would the presence of the attribute (without a change in behavior) make any difference?

Do any of the other participants have a response to @RalfJung 's point here? It definitely seems odd.

wesleywiser

wesleywiser commented on Nov 17, 2023

@wesleywiser
Member

If I'm reading this issue correctly, we have a msan instrumented C program compiled with clang 15 and a msan instrumented Rust binary compiled with Rust/LLVM 16. I believe in order for sanitizers in general to work correctly, all of the instrumented objects in your program need to be compiled against the same version of the sanitizer runtime.

Since this is somewhat expected as the sanitizer versions do not match, I'm going to close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    A-sanitizersArea: Sanitizers for correctness and code qualityC-bugCategory: This is a bug.P-highHigh priorityT-compilerRelevant to the compiler team, which will review and decide on the PR/issue.regression-untriagedUntriaged performance or correctness regression.

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @pnkfelix@RalfJung@wesleywiser@elichai@apiraino

        Issue actions

          Memory sanitizer : false positive regression in nightly-2023-01-18 · Issue #107149 · rust-lang/rust