Skip to content

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

Closed as not planned
Closed as not planned
Memory sanitizer : false positive regression in nightly-2023-01-18#107149
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.

Description

@catenacyber
catenacyber
opened on Jan 21, 2023

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

  1. catenacyber
    added
    C-bugCategory: This is a bug.
    regression-untriagedUntriaged performance or correctness regression.
    on Jan 21, 2023
  2. rustbot
    added
    I-prioritizeIssue needs a team member to assess the impact. Will be replaced by P-{low,medium,high,critical}
    on Jan 21, 2023

  3. catenacyber commented on Jan 21, 2023

    @catenacyber
    catenacyber
    on Jan 21, 2023
    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
``

  4. tmiasko commented on Jan 21, 2023

    @tmiasko
    tmiasko
    on Jan 21, 2023
    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.

  5. Noratrieb
    added
    A-sanitizersArea: Sanitizers for correctness and code quality
    on Jan 21, 2023
  6. catenacyber
    added a commit that references this issue on Jan 21, 2023

    suricata: fix rust+C MSAN build

    3fca5d8

  7. catenacyber commented on Jan 21, 2023

    @catenacyber
    catenacyber
    on Jan 21, 2023
    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
  8. DavidKorczynski
    added a commit that references this issue on Jan 21, 2023

    suricata: fix rust+C MSAN build (#9478)

    05c9ac2

  9. elichai commented on Jan 22, 2023

    @elichai
    elichai
    on Jan 22, 2023
    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?

  10. tcharding
    added 3 commits that reference this issue on Jan 22, 2023

    Overcome ASAN false positive regression

    4fe80cf

    Overcome ASAN false positive regression

    34d5c75

    Overcome ASAN false positive regression

    5a3f13e
  11. apoelstra
    added a commit that references this issue on Jan 23, 2023

    Merge #574: Overcome ASAN false positive regression

    0e689c7

  12. apiraino commented on Jan 25, 2023

    @apiraino
    apiraino
    on Jan 25, 2023
    Contributor

    WG-prioritization assigning priority (Zulip discussion).

    @rustbot label -I-prioritize +P-high

  13. 1 remaining item

  14. rustbot
    removed
    I-prioritizeIssue needs a team member to assess the impact. Will be replaced by P-{low,medium,high,critical}
    on Jan 25, 2023
  15. apiraino
    added
    T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.
    on Jan 25, 2023

  16. RalfJung commented on Jan 27, 2023

    @RalfJung
    RalfJung
    on Jan 27, 2023
    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?

  17. eamonnmcmanus
    added a commit that references this issue on Mar 15, 2023

    suricata: fix rust+C MSAN build (google#9478)

    df71bcd

  18. catenacyber commented on Mar 31, 2023

    @catenacyber
    catenacyber
    on Mar 31, 2023
    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 ?

  19. tmiasko commented on Mar 31, 2023

    @tmiasko
    tmiasko
    on Mar 31, 2023
    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.

  20. catenacyber commented on Apr 3, 2023

    @catenacyber
    catenacyber
    on Apr 3, 2023
    Author

    Thanks @tmiasko :-)

  21. DavidKorczynski
    added a commit that references this issue on Apr 4, 2023

    Suricata rust clang16 (#10019)

    bf6ebb8

  22. pnkfelix commented on Jun 30, 2023

    @pnkfelix
    pnkfelix
    on Jun 30, 2023
    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.

  23. wesleywiser commented on Nov 17, 2023

    @wesleywiser
    wesleywiser
    on Nov 17, 2023
    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.

  24. wesleywiser
    closed this as not plannedon Nov 17, 2023
  25. chain-forgexcr45
    added 2 commits that reference this issue on Sep 28, 2025

    Overcome ASAN false positive regression

    bf61336

    Merge rust-bitcoin/rust-secp256k1#574: Overcome ASAN false positive r…

    8916fef
  26. william2332-limf
    added 2 commits that reference this issue on Oct 2, 2025

    Overcome ASAN false positive regression

    34bb19d

    Merge rust-bitcoin/rust-secp256k1#574: Overcome ASAN false positive r…

    a9dcbeb
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

      Issue actions