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

CI should run Undefined Behavior Sanitizer (UBSAN), as already done for ASAN #111758

Open
thesamesam opened this issue Nov 5, 2023 · 6 comments
Labels
build The build process and cross-build type-feature A feature request or enhancement

Comments

@thesamesam
Copy link
Contributor

thesamesam commented Nov 5, 2023

Feature or enhancement

Proposal:

cpython's CI should run Undefined Behavior Sanitizer (UBSAN), like it already does for Address Sanitizer (ASAN).

It already has support in configure for this with ./configure --with-undefined-behavior-sanitizer.

This is important for portability, future proofing (keeping CPython working with a range of compilers), and avoiding confusing bug reports later when UB manifests.

Has this already been discussed elsewhere?

No response given

Links to previous discussion of this feature:

No response

@thesamesam thesamesam added the type-feature A feature request or enhancement label Nov 5, 2023
@thesamesam
Copy link
Contributor Author

thesamesam commented Nov 5, 2023

Note that --with-undefined-behavior-sanitizer does not set export UBSAN_OPTIONS=print_stacktrace=1:halt_on_error=1 right now.

By default, UBSAN will not abort on errors, which might mean errors are being missed by the fuzzer right now which does seem to use UBSAN? Not sure how the configuration works for that job:

$ grep -rsin undefined .github/
.github/workflows/build.yml:467:        sanitizer: [address, undefined, memory]

@thesamesam
Copy link
Contributor Author

cc @gpshead as we've discussed build issues before and this feels like it's in your wheelhouse

@gpshead
Copy link
Member

gpshead commented Nov 5, 2023

Overall in favor. Doing this in CI is predicated on if we actually pass UBSAN today and which things we need to categorize and disable in order to do so.

@gpshead
Copy link
Member

gpshead commented Nov 5, 2023

Note that there have been some UB reports in the past that may be technically correct from a C++ compiler spec perspective (read: clang, and note that C and C++ have different defined behavior definitions) but not actually undefined from a "giant swath of C code in the world has depended on the implemented behavior for 30+ years" perspective so the implemented behavior won't actually ever be changed. (see us passing -fwrapv to the compiler all over the place? defining that behavior in our build is one reason why)

Work to be done there (if any) is mostly around categorization and how to tell UBSAN which things we know and are okay with, the goal being not to introduce more.

@gpshead
Copy link
Member

gpshead commented Nov 5, 2023

For example, clang 17 ubsan gives triggers a build failure trying to run _freeze_module:

../cpython/Objects/object.c:2858:5: runtime error: call to function list_dealloc through pointer to incorrect function type 'void (*)(struct _o
bject *)'
../cpython/Objects/listobject.c:347: note: list_dealloc defined here

(*)(PyObject *) vs (*)(PyListObject *) is "undefined"? CPython casts things to PyObject* all over the place as a standard practice by design as a common C idiom for structure extension.

@thesamesam
Copy link
Contributor Author

thesamesam commented Nov 5, 2023

Yeah, see #111178 for that one. I'm not sure that this use as it stands is compatible with CFI which is a useful exploit mitigation.

But yeah, categorising existing errors sounds fine, sorting them into "things we should fix" and "things we want to make defined in CPython".

@iritkatriel iritkatriel added the build The build process and cross-build label Dec 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build The build process and cross-build type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

3 participants