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

cmd/cgo/internal/testsanitizers: TestMSAN/msan8 fails with clang16.0.6 #64616

Open
qiulaidongfeng opened this issue Dec 8, 2023 · 14 comments
Open
Assignees
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@qiulaidongfeng
Copy link
Contributor

Go version

go version devel go1.22-78b42a5338a Fri Dec 8 03:28:17 2023 +0000 linux/amd64

What operating system and processor architecture are you using (go env)?

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/root/.cache/go-build'
GOENV='/root/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/root/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/root/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/mnt/d/file/gofile/gogit/go1'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/mnt/d/file/gofile/gogit/go1/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='devel go1.22-78b42a5338a Fri Dec 8 03:28:17 2023 +0000'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='clang'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/mnt/d/file/gofile/gogit/go1/src/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build9589947017406823330=/tmp/go-build -gno-record-gcc-switches'
GOROOT/bin/go version: go version devel go1.22-78b42a5338a Fri Dec 8 03:28:17 2023 +0000 linux/amd64
GOROOT/bin/go tool compile -V: compile version devel go1.22-78b42a5338a Fri Dec 8 03:28:17 2023 +0000
uname -sr: Linux 5.15.133.1-microsoft-standard-WSL2
Distributor ID: Kali
Description:    Kali GNU/Linux Rolling
Release:        2023.4
Codename:       kali-rolling
/lib/x86_64-linux-gnu/libc.so.6: GNU C Library (Debian GLIBC 2.37-12) stable release version 2.37.

What did you do?

In wsl2
export CC=clang
clang --version
Debian clang version 16.0.6 (16)
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin

go test cmd/cgo/internal/testsanitizers

What did you expect to see?

test pass.

What did you see instead?

--- FAIL: TestMSAN (9.40s)
    --- FAIL: TestMSAN/msan8 (3.26s)
        msan_test.go:81: /tmp/TestMSAN1734687677996663673/msan8 exited with exit status 1
            ==29491==WARNING: MemorySanitizer: use-of-uninitialized-value
                #0 0x5062b8 in msanGoLoop (/tmp/TestMSAN1734687677996663673/msan8+0x5062b8) (BuildId: 519103c2e3c209c99d8a9bb8f9d7cf2065339178)
                #1 0x5064c0 in _cgo_a322d3c7e754_Cfunc_msanGoLoop (/tmp/TestMSAN1734687677996663673/msan8+0x5064c0) (BuildId: 519103c2e3c209c99d8a9bb8f9d7cf2065339178)
                #2 0x501143 in runtime.asmcgocall.abi0 /mnt/d/file/gofile/gogit/go1/src/runtime/asm_amd64.s:918

            SUMMARY: MemorySanitizer: use-of-uninitialized-value (/tmp/TestMSAN1734687677996663673/msan8+0x5062b8) (BuildId: 519103c2e3c209c99d8a9bb8f9d7cf2065339178) in msanGoLoop
            Exiting
FAIL
FAIL    cmd/cgo/internal/testsanitizers 32.323s
FAIL
@mauri870 mauri870 added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. compiler/runtime Issues related to the Go compiler and/or runtime. labels Dec 8, 2023
@mauri870
Copy link
Member

mauri870 commented Dec 8, 2023

This appears to be a general clang 16 issue, not only to wsl2.

It is complaining about the use of uninit that is uninitialized here:

https://go.googlesource.com/go/+/78b42a5338aa1fa293acc5bbb7ef9122a7acc2ba/src/cmd/cgo/internal/testsanitizers/testdata/msan8.go#65

Shorter repro:

package main

/*
#include <stdint.h>

#include <sanitizer/msan_interface.h>

__attribute__((noinline))
void funinit(unsigned long a1) {}

static unsigned long uninit;

void f() {
	__msan_poison(&uninit, sizeof uninit);
	funinit(uninit);
}
*/
import "C"

func main() {
	C.f()
}

Strangely enough I can't seem to replicate it with a C version and clang-16.

@mauri870 mauri870 added help wanted NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Dec 9, 2023
@mauri870
Copy link
Member

mauri870 commented Dec 11, 2023

Huh, something appear to have changed in clang-16 that makes it report memory as uninitialized in this case. From the test msan8 it reads:

// msanGoLoop loops calling msanGoWait, with the arguments passed
// such that msan thinks that they are undefined. msan permits
// undefined values to be used as long as they are not used to
// for conditionals or for memory access.

I tried clang 14, 15, 16, 17 and this only happens with clang >= 16. Looking at the release notes I can't spot anything that might've caused this change in msan behavior https://releases.llvm.org/16.0.0/docs/ReleaseNotes.html

I'm not sure if this comment above is actually a guarantee from the memory sanitizer, they might have changed it silently in clang-16 without mentioning it.

Here is a short C repro I put together that runs in clang 15 but not on clang 16:

// clang-16 -fsanitize=memory test.c && ./a.out
#include <stdint.h>
#include <sanitizer/msan_interface.h>

__attribute__((noinline))
void funinit(unsigned long a1) {}

static unsigned long uninit ;

void f() {
	__msan_poison(&uninit, sizeof uninit);
	funinit(uninit);
}

int main() {
    f();
    return 0;
}

@mauri870 mauri870 added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed help wanted NeedsFix The path to resolution is known, but the work has not been done. labels Dec 11, 2023
@mauri870
Copy link
Member

Adjusting the labels as this appears to be a clang issue

@mauri870
Copy link
Member

cc @dvyukov

@bcmills bcmills changed the title cmd/cgo/internal/testsanitizers: test fail in wsl2 with clang16.0.6 cmd/cgo/internal/testsanitizers: test fails with clang16.0.6 Dec 11, 2023
@bcmills bcmills changed the title cmd/cgo/internal/testsanitizers: test fails with clang16.0.6 cmd/cgo/internal/testsanitizers: TestMSAN/msan8 fails with clang16.0.6 Dec 11, 2023
@dvyukov
Copy link
Member

dvyukov commented Dec 12, 2023

This looks like -fsanitize-address-param-retval (detects passing uninits as arguments), but it should be disabled by default. Maybe it was accidentally enabled in clang 16 and then disabled.
@kda @vitalybuka

@vitalybuka
Copy link

It's enabled by default https://reviews.llvm.org/D134669

@vitalybuka
Copy link

@mauri870
Copy link
Member

Looks like that is the culprint, I was looking at the llvm release notes and not the tools, that is why I did not spot this change.

Thanks for the valuable insight @vitalybuka @dvyukov.

@mauri870 mauri870 added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Dec 12, 2023
@mauri870
Copy link
Member

@bcmills Do you consider this as a breaking change? This new behavior seems to be beneficial for programs:

-fsanitize-memory-param-retval is turned on by default. With -fsanitize=memory, passing uninitialized variables to functions and returning uninitialized variables from functions is more aggressively reported. -fno-sanitize-memory-param-retval restores the previous behavior.

Should we retain the old behavior?

@dvyukov
Copy link
Member

dvyukov commented Dec 12, 2023

I think end users can control this using CGO_CFLAGS. So the breakage is not that bad.
And potentially we can go for the better/default behavior and gave users workaround via CGO_CFLAGS.

@vitalybuka
Copy link

I think end users can control this using CGO_CFLAGS. So the breakage is not that bad. And potentially we can go for the better/default behavior and gave users workaround via CGO_CFLAGS.

The state of that flag for linked C/C++ code needs to be consistent with GO code. Mismatch either way will cause false positives.

@gopherbot
Copy link

Change https://go.dev/cl/549297 mentions this issue: cmd/cgo/internal/testsanitizers: fix msan test failing with clang >= 16

@mknyszek mknyszek added this to the Backlog milestone Dec 13, 2023
@bcmills bcmills reopened this Dec 18, 2023
@mauri870
Copy link
Member

So it appears that we cannot use #if with #cgo, so the fix I submitted was reverted. I'm glad to hear suggestions on how to handle this.

@bcmills
Copy link
Contributor

bcmills commented Jan 4, 2024

@mauri870, perhaps the test driver itself could first try building with CGO_CFLAGS=-fno-sanitize-memory-param-retval, and try again without that flag if the build fails?

ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
Clang 16 introduced a more aggressive behavior regarding uninitialized
memory in the memory sanitizer.

The new option -fsanitize-memory-param-retval is enabled by default
and makes the test msan8 fail, since it uses an
uninitialized variable on purpose.

Disable this behavior if we are running with clang 16+.

Fixes golang#64616

Change-Id: If366f978bef984ea73f6ae958f24c8fce99b59fe
GitHub-Last-Rev: 60bd64a
GitHub-Pull-Request: golang#64691
Reviewed-on: https://go-review.googlesource.com/c/go/+/549297
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Than McIntosh <thanm@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Projects
Status: In Progress
Development

Successfully merging a pull request may close this issue.

7 participants