Skip to content

./configure --with-address-sanitizer adds -fsanitize flags to CFLAGS/LDFLAGS used by 3rd party C extensions #109575

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

Closed
vstinner opened this issue Sep 19, 2023 · 7 comments

Comments

@vstinner
Copy link
Member

vstinner commented Sep 19, 2023

./configure --with-address-sanitizer (ASAN) and similar options for MSAN and UBSAN adds compiler and linker options not only to build Python itself, but also to build 3rd party C extensions.

The flags should be added to "NODIST" variables instead.

Linked PRs

vstinner added a commit to vstinner/cpython that referenced this issue Sep 19, 2023
When Python is configured to use ASAN, MSAN or UBSAN sanitizer, with
"./configure --with-address-sanitizer" for example, compiler and
linker flags for sanitizers are no longer exported to third party C
extensions. Add flags to CFLAGS_NODIST and LDFLAGS_NODIST, instead of
BASECFLAGS and LDFLAGS.
@vstinner
Copy link
Member Author

Example of the issue:

$ ./configure --with-pydebug --with-address-sanitizer CC=clang 
$ grep fsanitize Makefile
BASECFLAGS=	-fsanitize=address -fno-omit-frame-pointer  -fno-strict-overflow -Wsign-compare -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer
CONFIGURE_LDFLAGS=	-fsanitize=address 

I wrote PR #109576 to fix the issue.

vstinner added a commit to vstinner/cpython that referenced this issue Sep 19, 2023
When Python is configured to use ASAN, MSAN or UBSAN sanitizer, with
"./configure --with-address-sanitizer" for example, compiler and
linker flags for sanitizers are no longer exported to third party C
extensions. Add flags to CFLAGS_NODIST and LDFLAGS_NODIST, instead of
BASECFLAGS and LDFLAGS.

Makefile.pre.in: PY_LDFLAGS_NOLTO now uses PY_LDFLAGS_NODIST, instead
of LDFLAGS_NODIST.
vstinner added a commit to vstinner/cpython that referenced this issue Sep 19, 2023
When Python is configured to use ASAN, MSAN or UBSAN sanitizer, with
"./configure --with-address-sanitizer" for example, compiler and
linker flags for sanitizers are no longer exported to third party C
extensions. Add flags to CFLAGS_NODIST and LDFLAGS_NODIST, instead of
BASECFLAGS and LDFLAGS.

Makefile.pre.in: PY_LDFLAGS_NOLTO now uses PY_LDFLAGS_NODIST, instead
of LDFLAGS_NODIST, and add CONFIGURE_LDFLAGS_NOLTO after
PY_LDFLAGS_NODIST.
@gpshead
Copy link
Member

gpshead commented Sep 20, 2023

I don't know how much this matters.

I could also imagine people who wanted the flags exported for doing full builds of everything with selected sanitizers available?

Do we know of any distros that ship any sanitizer enabled builds as packages?

Or of people building a CPython configured this way as part of a larger CI test that then builds extensions to test?

@vstinner
Copy link
Member Author

My experience with sanitizers is not great. Unless you have a whole system (!) built with sanitizers, you are quickly getting troubles with some libraries which are not aware of sanitizers.

OpenSSL and MSAN: openssl/openssl#17784

I strongly suspect it is a false positive caused by the fact that the openssl library you're linking against is not built with -fsanitize=memory.

libc stat() and MSAN: #91043 and llvm/llvm-project#54131

It's unclear to me if it's an issue in Python itself, Clang/GCC, or how the glibc was built. (...) Anyway, I close the issue.

@gpshead:

Do we know of any distros that ship any sanitizer enabled builds as packages?

I'm not aware of any Linux distribution shipping packages built with ASAN, MSAN, UBSAN or another sanitizer. It seems like it's more developers building their softwares with sanitizers, but try to ignore warnings/errors in other projects code.


Just building Python with ASAN quickly crash since ASAN thinks that Python is leaking memory. I'm not even talking about running any trivial code, just build Python.

git clean -fdx
./configure --with-address-sanitizer CC=clang
make SHELL="bash -x"

Output:

(...)
+ ./_bootstrap_python ./Programs/_freeze_module.py importlib.util ./Lib/importlib/util.py Python/frozen_modules/importlib.util.h
./_bootstrap_python ./Programs/_freeze_module.py runpy ./Lib/runpy.py Python/frozen_modules/runpy.h
+ ./_bootstrap_python ./Programs/_freeze_module.py importlib.machinery ./Lib/importlib/machinery.py Python/frozen_modules/importlib.machinery.h
+ ./_bootstrap_python ./Programs/_freeze_module.py runpy ./Lib/runpy.py Python/frozen_modules/runpy.h

=================================================================
==819456==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 20202 byte(s) in 412 object(s) allocated from:
    #0 0x4d1ca2 in __interceptor_malloc (/home/vstinner/python/main/_bootstrap_python+0x4d1ca2) (BuildId: 75c20068b0ade2526ba63d8eaeca3c90445e197a)
    #1 0x75de2d in PyUnicode_New /home/vstinner/python/main/Objects/unicodeobject.c:1244:24
    #2 0x762baa in _PyUnicode_FromUCS1 /home/vstinner/python/main/Objects/unicodeobject.c:2031:11
    #3 0x762baa in PyUnicode_FromKindAndData /home/vstinner/python/main/Objects/unicodeobject.c:2102:16
    #4 0x9b0f1d in r_object /home/vstinner/python/main/Python/marshal.c:1157:17
    #5 0x9b0b8a in r_object /home/vstinner/python/main/Python/marshal.c:1222:18
(...)

You have to explicitly tune ASAN to disable checking for memory leaks. Python Buildbot and GHA CI jobs use more configuration:

ASAN_OPTIONS="detect_leaks=0:allocator_may_return_null=1:handle_segv=0"

@vstinner
Copy link
Member Author

I could also imagine people who wanted the flags exported for doing full builds of everything with selected sanitizers available?

As an user, I would prefer to select which C extensions are built with the sanitizer.

@sunmy2019
Copy link
Member

you are quickly getting troubles with some libraries which are not aware of sanitizers.

ASAN doesn't require all parts to be built with asan. MSAN does require.

@gpshead
Copy link
Member

gpshead commented Sep 20, 2023

Indeed, in the MSAN use case we should continue propagating the flag as the only time someone is doing an MSAN build is if they're building a whole instrumented stack (loading non-MSAN instrumented code into an MSAN process will lead to MSAN failures without difficult to manage ignorelists setup beforehand).

Agreed that the others do not necessarily need to be propagated as UBSAN and ASAN are a lot closer to being stand alone instrumentation.

As a user, I would prefer to select which C extensions are built with the sanitizer.

Do we have non-core-dev users complaining about this?

Is sanitizer flag propagation to third party extension module building causing problems?

What's the motivation for changing it beyond feeling like it might be wrong?

@vstinner vstinner closed this as completed Oct 2, 2023
@vstinner
Copy link
Member Author

vstinner commented Oct 2, 2023

I closed my PR: #109576 (comment)

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