-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
gh-132097: allow AC to disable fastcall convention to avoid UBSan failures #131605
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR changes two things: remove cast and add @disable
. It changes 119 files so it's hard to review. You should split your PR int two PRs.
I'm not excited by the removal of the cast since it impacts tons of files (all of them?). I'm +0 on it.
It helped me for grepping contents but I'll cancel that one in this PR and make it in a different one (after everything is done). Indeed, let's focus on actually fixing something. |
The PR title seems to be outdated. |
It seems to me that rather than disabling fastcall, what's needed here is to force a particular calling convention. That is, if Clinic changes from fastcall to something else as the new best way of doing things, these functions will still need to stay
Disclaimer: I haven't looked into Clinic to check how hard this would be to implement; I'm only commenting at the “API level”. |
That's what I first thought about but as I'm really not familiar with clinic and its assumptions, it was faster to disable fastcall explicitly as there is a |
It seems like test_clinic pass on Python built with |
When I tested this, there was a runtime UB that I caught. I've used: Note that I've caugh the UB when running the entire test suite, so I don't know if it was |
I can confirm that I still have the UB on main: ./python -m test test_clinic
Using random seed: 1409063504
0:00:00 load avg: 9.45 Run 1 test sequentially in a single process
0:00:00 load avg: 9.45 [1/1] test_clinic
Objects/methodobject.c:551:18: runtime error: call to function posonly_poskw_varpos_no_fastcall through pointer to incorrect function type 'struct _object *(*)(struct _object *, struct _object *, struct _object *)'
/$HOME/lib/python/cpython/./Modules/clinic/_testclinic.c.h:4174: note: posonly_poskw_varpos_no_fastcall defined here
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior Obje My clang version:
The ./configure --with-undefined-behavior-sanitizer --with-pydebug --prefix="$(pwd)/build" CC=clang LD=clang CFLAGS="-fsanitize=undefined -fno-sanitize-recover" LDFLAGS="-fsanitize=undefined -fno-sanitize-recover" |
Note that I still have a UB on ./python -m test test_xml_etree_c
Using random seed: 644741740
0:00:00 load avg: 3.09 Run 1 test sequentially in a single process
0:00:00 load avg: 3.09 [1/1] test_xml_etree_c
Modules/expat/xmlparse.c:6779:5: runtime error: call to function expat_default_handler through pointer to incorrect function type 'void (*)(void *, const char *, int)'
/$HOME/lib/python/cpython/./Modules/_elementtree.c:3212: note: expat_default_handler defined here
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior Modules/expat/xmlparse.c:6779: I thought you fixed this one? (I can comment on the XML PR if you prefer) EDIT: Fixed in #132265. |
Aha, I only tested |
Yes, I can |
A new extension module, `_hmac`, now exposes the HACL* HMAC (formally verified) implementation. The HACL* implementation is used as a fallback implementation when the OpenSSL implementation of HMAC is not available or disabled. For now, only named hash algorithms are recognized and SIMD support provided by HACL* for the BLAKE2 hash functions is not yet used.
0b34b63
to
24475b1
Compare
This comment was marked as resolved.
This comment was marked as resolved.
24475b1
to
65c64bc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
We need a way to explicitly disable fastcall convention for non-new methods. Let me explain why. Consider the following case:
What clinic generates is two signatures:
At runtime, the function being called will be
varpos_no_fastcall
. In particular, we need to make it aMETH_VARARGS | METH_KEYWORD | METH_CLASS
. However, and this is where the runtime UB happens, the fact that we are havingMETH_VARARGS
inmethodobject.c::cfunction_call()
implies that it will be used as follows:The important bits are:
As you can see,
self
is being passed as aPyObject *
to the called method. But, here,meth
isvarpos_no_fastcall
, which expects aPyTypeObject *
instead, hence the undefined behaviour. Now, the unfortunate part is that we must use all flagsMETH_VARARGS | METH_KEYWORD | METH_CLASS
here.One could wonder: why not using something else as a base classmethod instead of
__new__
? well.. the answer is that if a classmethod that is not__new__
is generated by AC, then that method will use aMETH_FASTCALL
convention, which entirely defeats the purpose of the test.For efficiency purposes (and that's what we want obviously), if we use
AC will generate:
and we will also have some autogenerated
METHODDEF
macro containingMETH_FASTCALL|METH_CLASS
flags. We also cannot just usevarpos_no_fastcall
without theMETH_FASTCALL
flag as its signature wouldn't be compatible forcfunction_call
which expects either of the two forms:So the solution I came up with is a generic
@disable
directive that can disable the fastcall convention explicitly even for non-__new__
class methods:AC will generate:
and a
METHODDEF
macro withMETH_VARARGS|METH_CLASS
. Note that nowvarpos_no_fastcall
has the expected signature and can be correctly called at runtime bycfunction_call
. I've explained all of this in the comments of the tested functions (_testclinic.c
). Since it's part of the tests, I think we can safely keep those comments.-fsanitize=undefined -fno-sanitize-recover
#132097