Repository navigation
Undefined behaviour in main and 3.11 #96678
Description
Activity
- changed the title
[-]Undefined behaviours in main and 3.11[/-][+]Undefined behaviours in `main` and 3.11[/+]on Sep 8, 2022 - changed the title
[-]Undefined behaviours in `main` and 3.11[/-][+]Undefined behaviour in `main` and 3.11[/+]on Sep 8, 2022 Can you post a summary of "arithmetic with NULL pointers" here? Thanks
@kumaraditya303 In
ceval.c, theassertis my addition./* Pack other positional arguments into the *args argument */ if (co->co_flags & CO_VARARGS) { PyObject *u = NULL; assert(args != NULL); u = _PyTuple_FromArraySteal(args + n, argcount - n); if (u == NULL) { goto fail_post_positional; } assert(localsplus[total_args] == NULL); localsplus[total_args] = u; }
There's also an example in
Modules/_testcapimodule.cbut that's only a test, I guess.#96672 is also vaguely related.
cc @pablogsal This seems to be introduced in #89419
I am a bit surprised by this because we have a USAN buildbot that has not detected anything:
These are the parameters, just in case we want to compare:
@pablogsal I added some more info about what compiler I am using.
@pablogsal I added some more info about what compiler I am using.
Can you see if you reproduce these warnings using the exact flags the buildbot is using?
I'll try that later today.
33 remaining items
As a practical test, we can take the asserts I added in the PRs mentioned in the text of the issue, convert them into something that triggers in release mode (eg a normal
ifand a printf or abort) and run them under the same conditions as release mode.I don't know why the sanitizer doesn't pick any of this up. At least it doesn't pick any of this up when the build bot runs it, neither in release mode nor in the debug mode.
I ran my sanitizer locally with different flags. See above.
(I'm only on my phone right now, so can't run these experiments until later today.)
I don't know why the sanitizer doesn't pick any of this up. At least it doesn't pick any of this up when the build bot runs it, neither in release mode nor in the debug mode.
Answering that question is very important, as it may reveal very interesting things such as problems in the running builder or what is the difference in behaviour between your builds and the builder and if the differences matte or not.
neither in release mode nor in the debug mode.
The sanitizer only runs in release mode in the build bots
The sanitizer only runs in release mode in the build bots
Oh, thanks. I must have misunderstood something.
Of course, our general point about not actually picking up many instances of UB still stands.
Btw, the config that you linked to that had the sanitizer enabled didn't enable optimisations as far as I can tell, hence I didn't see it as release mode.
@pablogsal @markshannon @kumaraditya303
Good news! I found the rootcause of why my debug build with all sanitisers turned up showed undefined behaviour, but they didn't show up in the build bot.
Most invocations of
configureadd-fwrapvto the compiler options. But--with-pydebugremoves that.From clang's documentation about
-fwrapvTreat signed integer overflow as two’s complement
GCC is a bit more verbose, but still rather vague:
This option instructs the compiler to assume that signed arithmetic overflow of addition, subtraction and multiplication wraps around using twos-complement representation. This flag enables some optimizations and disables others.
It turns out that arithmetic on the null-pointer also becomes defined with
-fwrapv. At least my experiments point in that direction. The only thing I could find is some discussion on the netbsd kernel mailing list that seems to confirm this.If the above is true, then all the instances of UB that I found are not actually UB in the effective C-dialect we are using for release builds.
Now we have (at least) two options:
- fix
--with-pydebugto also add-fwrapvfor consistency. - remove
-fwrapvfrom the release builds.
The former is the minimal change and safe. The latter enables a lot of C compiler optimizations, and my experiments running the sanitisers suggests that we are already nearly
-fno-wrapv-ready. In general, we might want to look into-fstrict-overflow, too.I say we should do the former straight away, and in the longer term consider implementing the latter (after running benchmarks and weighing the pros and cons etc).
- fix
- added a commit that references this issue
on Sep 13, 2022 - added 4 commits that reference this issue
on Sep 13, 2022 I am closing this issue for now, because thanks to
-fwrapvwe don't actually have any UB in the release builds. Only when building--with-pydebugor with an explicit optimization option on the command line.
I ran the sanitizers again, and found a few more instances of undefined behaviour, mostly around bit-shifting of signed integers and arithmetic with NULL pointers.
I put some asserts to demonstrate the undefined behaviour into pull requests for main (matthiasgoergens#18) and 3.11 (matthiasgoergens#19).
More information about my environment: