Open mdboom opened 9 months ago
colesbury
commented
9 months ago I don't think we want -fstrict-flex-arrays=3. We need flexible array members and we need C++ support, so we're forced to rely on the (widely supported) compiler extension of using field[0] or field[1] as a flexible array member.
sobolevn
commented
9 months ago Some are probably un-resolvable (format-literal is pretty hard to avoid when wrapping sprintf, for example)
These warnings do no make much sense in current use-cases:
Objects/unicodeobject.c:2592:21: warning: format not a string literal, argument types not checked [-Wformat-nonliteral]
2592 | sprintf(buffer, fmt, va_arg(*vargs, long)) :
| ^~~~~~~
Objects/unicodeobject.c:2593:21: warning: format not a string literal, argument types not checked [-Wformat-nonliteral]
2593 | sprintf(buffer, fmt, va_arg(*vargs, unsigned long));
| ^~~~~~~
Objects/unicodeobject.c:2597:21: warning: format not a string literal, argument types not checked [-Wformat-nonliteral]
2597 | sprintf(buffer, fmt, va_arg(*vargs, long long)) :
| ^~~~~~~
Objects/unicodeobject.c:2598:21: warning: format not a string literal, argument types not checked [-Wformat-nonliteral]
2598 | sprintf(buffer, fmt, va_arg(*vargs, unsigned long long));
| ^~~~~~~
Objects/unicodeobject.c:2602:21: warning: format not a string literal, argument types not checked [-Wformat-nonliteral]
2602 | sprintf(buffer, fmt, va_arg(*vargs, Py_ssize_t)) :
| ^~~~~~~
Objects/unicodeobject.c:2603:21: warning: format not a string literal, argument types not checked [-Wformat-nonliteral]
2603 | sprintf(buffer, fmt, va_arg(*vargs, size_t));
| ^~~~~~~
Objects/unicodeobject.c:2606:17: warning: format not a string literal, argument types not checked [-Wformat-nonliteral]
2606 | len = sprintf(buffer, fmt, va_arg(*vargs, ptrdiff_t));
| ^~~
Objects/unicodeobject.c:2610:21: warning: format not a string literal, argument types not checked [-Wformat-nonliteral]
2610 | sprintf(buffer, fmt, va_arg(*vargs, intmax_t)) :
| ^~~~~~~
Objects/unicodeobject.c:2611:21: warning: format not a string literal, argument types not checked [-Wformat-nonliteral]
2611 | sprintf(buffer, fmt, va_arg(*vargs, uintmax_t));
| ^~~~~~~
Objects/unicodeobject.c:2615:21: warning: format not a string literal, argument types not checked [-Wformat-nonliteral]
2615 | sprintf(buffer, fmt, va_arg(*vargs, int)) :
| ^~~~~~~
Objects/unicodeobject.c:2616:21: warning: format not a string literal, argument types not checked [-Wformat-nonliteral]
2616 | sprintf(buffer, fmt, va_arg(*vargs, unsigned int));
| ^~~~~~~I think that they should be silenced / ignored.
sethmlarson
commented
9 months ago @mdboom Are you okay with me editing your topic to create a checklist style table with links to either why we're not implementing or the actual implementation? My guess is we'll be adopting these one by one :)
mdboom
commented
9 months ago @mdboom Are you okay with me editing your topic to create a checklist style table with links to either why we're not implementing or the actual implementation? My guess is we'll be adopting these one by one :)
@sethmlarson: Good idea.
hugovk
commented
9 months ago At a high level, I think the process to address these and make incremental progress maybe looks something like:
- Pick one of the warning types, and assess how many false positives it gives and how onerous it is to fix them. From this, build concensus about whether it's worth addressing.
- Fix all of the existing instances.
- Turn that specific warning into an error so it doesn't creep back in.
Sounds a good approach.
To share another method that could additionally help: as part of https://github.com/python/cpython/issues/101100, we're working through a lot of docs "nit-picky" warnings.
When building the docs, we only allow warnings to occur in files that already have warnings and are listed in a .nitignore file. Once a file has been cleaned, we remove it from the list to prevent regressions.
We also fail the docs build if we "accidentally" clean a file: if warnings do not occur in a file where we previously expected warnings, so the file must also be removed from the list, again to prevent regressions.
This does need some custom tooling, but it's helped us make gradual progress, and we've fixed 40% so far.
carsonRadtke
commented
9 months ago RE: @hugovk's .nitignore
I am in favor of a solution like this. It would not require any custom tooling as we could change the build arguments to whatever we find consensus in and then silence compiler warnings for offending lines until somebody comes along and fixes them.
This also allows us to silence errors locally, but enforce them globally. That way we could still have -Wformat-nonliteral, but allow non-compliance during the compilation of 'unicodeobject.c'. (I am not advocating for this flag, just using it as an example)
nohlson
commented
3 months ago Hello all I have been selected by GSoC to work on this!
@mdboom I am curious how you were able to get the unit tests to pass with the linker option -Wl,-z,nodlopen. I am testing options out on my own machine Linux/x86_64/gcc.
configure determines that because dlopen() is available that dynload_shlib.o should be linked which uses dlopen(), so later on when importing modules I understandably get an error that shared objects cannot be dlopen()ed. I'm not even aware of any alternative so I was curious how you were able to avoid dlopen() and pass tests that load modules
mdboom
commented
3 months ago Welcome, @nohlson! I was really excited to hear about this GSoC project at PyCon.
This whole investigation for me was a quick afternoon hack. I only ever got as far as getting the build to complete -- I never even ran Python, let alone its test suite. I just got as far as thinking "someone with more time should work on this", and here you are ;)
Also looking at this again, I see I didn't set the linker flags on LDFLAGS, only CFLAGS, and therefore it seems they had no effect. So there's nothing magical about it working for me and not you.
IMHO, this seems like a hard flag to support. "Python without dlopen" would be a different beast -- maybe some very security conscious people would want that, but it would require tooling to statically link in all expected extension modules (some of that tooling already exists elsewhere). So, personally I'd defer solving that one for now (but I'm not the GSoC mentor, that's just my opinion).
nohlson
commented
2 months ago I would like to get some discussion going about performance impacts of enabling options and how much we would be willing to conceed in performance for safety.
Here is an example of a cypthon baseline pyperformance benchmark vs. a build with multiple performance-impacting options enabled broke down by benchmark category:
| Benchmark Tag | Geometric Mean |
|---|---|
| apps | 1.03x slower |
| asyncio | 1.04x slower |
| math | 1.03x slower |
| regex | 1.00x faster |
| serialize | 1.08x slower |
| startup | 1.03x slower |
| template | 1.03x slower |
| template | 1.04x slower |
To see all the benchmarks run and the comparison between builds here is more detail:
I am putting together some analysis of how individual options affect performance that I will share but would like to get some optionions concerning which benchmarks can't afford to take a performance hit. For example I would be less concerned about startup and docutils benchmarks as regex and math benchmarks which could be used at high frequency in applications.
mdboom
commented
2 months ago I think, unfortunately, the answer to that is "it depends". Startup really matters for some applications, and not others, for example. Likewise, security really matters in some contexts, but not others. It's hard to speculate at the beginning of this project, but maybe the end result will be to make it easy to make a security-hardened build at the expense of performance when the end user wants to make that tradeoff.
I like the idea of breaking this out by individual flags, so we can see which have the most impact. It might also be possible that we can reduce the impact of some of the options by changing how some code is written in CPython, i.e. if an option makes some unsafe C feature slower, maybe we try to stop using that unsafe C feature if we can ;)
We have a whole set of standard benchmarking machines at Microsoft that are set up to get the results as-reproducible-as-possible. If you create a branch on your fork of CPython with some proposed changes, you can ping me and I can kick off a run, and the results will show up automatically on iscpythonfastyet.com. Unfortunately, we can't automate triggering those runs for security reasons, but it's really easy for me so don't hesitate to ask me and I can get to it pretty quickly during my working hours.
corona10
commented
2 months ago @nohlson By the way, fail-through emits a lot of warnings from the CPython build (we pursue remove compiler warnings as possible), and some of them are intended fail-throughs. Do you have any plans for this?
./Modules/_testcapi/exceptions.c:38:9: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
case 2:
^
./Modules/_testcapi/exceptions.c:38:9: note: insert '__attribute__((fallthrough));' to silence this warning
case 2:
^
__attribute__((fallthrough));
./Modules/_testcapi/exceptions.c:38:9: note: insert 'break;' to avoid fall-through
case 2:
^
break;
./Modules/_testcapi/exceptions.c:42:9: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
case 1:
^
./Modules/_testcapi/exceptions.c:42:9: note: insert '__attribute__((fallthrough));' to silence this warning
case 1:
^
__attribute__((fallthrough));
./Modules/_testcapi/exceptions.c:42:9: note: insert 'break;' to avoid fall-through
case 1:
^
break; And for the expat module, you should send patches to the upstream. https://github.com/libexpat/libexpat cc @hartwork
nohlson
commented
2 months ago @corona10 Yes I am going to be implementing some tooling to keep track of new warnings that are generated by enabling these new flags, which is the next step of this process.
I had actually overlooked those warnings until I saw them in some of the buildbot compile logs. I had intended for the first round to be warning-free.
We can start by deciding if we should ignore the intended fall-through warnings in the tooling or add the attributes.
corona10
commented
2 months ago We can start by deciding if we should ignore the intended fall-through warnings in the tooling or add the attributes.
Then, how about we revert the fall-through warning changes and then just test when your new tool is implemented? (I like your idea, but we need to separate tasks as possible) If not until then, emitted warnings(and as I said, some codes are vendored modules, not ours; we should submit the patch to the upstream first, which the fix can be delayed until they release new version) will be stressful for some core devs.
nohlson
commented
2 months ago @corona10 I agree let's remove fall-through warnings
and I will look into why I am not seeing those warnings when I build locally
nohlson
commented
2 months ago @corona10 PR to remove fallthrough warning option: https://github.com/python/cpython/pull/121041
Just from browsing the builds from #121030 it seems that clang is only emitting warnings for fallthrough.
Here are builds with warnings: https://buildbot.python.org/all/#/builders/721/builds/1465 (macos) https://buildbot.python.org/all/#/builders/111/builds/1387 (Fedora with clang)
And others don't (Fedora/RHEL w/ gcc): https://buildbot.python.org/all/#/builders/816/builds/977 https://buildbot.python.org/all/#/builders/745/builds/987 https://buildbot.python.org/all/#/builders/115/builds/1398
I will pay extra close attention to these compiler nuances when working on the tooling.
nohlson
commented
2 months ago @hugovk For the warning tooling I had initially considered if it would be feasible to add to the pipeline for each of the buildbots maybe even as a unit test the warning checks, but keeping track of the warnings for each platform/compiler might be too complicated. I was considering just making a couple github actions that run the warning check tooling for macos, ubuntu, and windows and that would be representative, just as was done for the docs warning tracker.
Are there any thoughts on the latter approach?
hugovk
commented
2 months ago Sounds like a good idea to just test on a subset. Using GitHub Actions means we can run as part of all PRs, and also people can test on their forks without too much bother.
Things to consider: do we want warnings to fail the build, so that people can't merge if they introduce new warnings? If not now, we can consider this for later.
The next level is allow warnings to report as a failure, but not let that failure block a merge. A downside of this is that it can still shows as red, even if they can merge, which people find annoying.
Then the next level is just to output the warnings in the log, and the job always reports as passed. The downside is you need to dig into the logs to see what happened, and most people won't do that.
If we're still at the investigation phase, we probably don't want to fail PRs just yet, but I imagine at some point we will.
Another thing to consider: will this be new jobs, or something added to the existing build?
For the docs warnings, we added it to an existing build: output all warnings to log, but otherwise build as usual, then in a later step of the same job, run a script to analyse the logs and decide when to fail.
nohlson
commented
2 months ago @hugovk Awesome thank you for the input! I am moving forward with a GitHub Actions solution.
Things to consider: do we want warnings to fail the build, so that people can't merge if they introduce new warnings? If not now, we can consider this for later.
The first iteration I will introduce will have options at the script level for failing on regression or improvement just as the docs version has but will have both disabled in the github action configuration. The results of the checks will just be printed out as a "warning" about warnings
The next level is allow warnings to report as a failure, but not let that failure block a merge. A downside of this is that it can still shows as red, even if they can merge, which people find annoying.
Once the tooling has been introduced we can create a new PR that enables an option that does produce a small number of new warnings. At this point we could enable fail on regression. Then we can decide if we are going to add to the ignore list or fix the manageable number of warnings.
Another thing to consider: will this be new jobs, or something added to the existing build?
Currently I am going to try to fit this into the existing ubuntu build job. Then I will take a look at the macos and windows jobs after. The design is very similar to the docs warnings checks.
hugovk
commented
2 months ago Sounds good 👍
hugovk
commented
2 months ago First PR for this: https://github.com/python/cpython/pull/121730 🚀
kulikjak
commented
1 month ago Hi, it seems that Solaris/Illumos linker is unhappy when you pass -fstack-protector-strong via into CFLAGS but not LDFLAGS, resulting in the following failure after #120975:
Undefined first referenced
symbol in file
__stack_chk_fail Programs/_freeze_module.o
__stack_chk_guard Programs/_freeze_module.o
ld: fatal: symbol referencing errorsI created #121979 to resolve this issue.
encukou
commented
1 month ago Adding the fortify source level 3 flag broke test_cext and test_cppext on several buildbots, e.g. here: https://buildbot.python.org/#/builders/64/builds/7269/steps/6/logs/stdio
Please fix or revert.
Building wheels for collected packages: internal-test-limited-c11-cext
Building wheel for internal-test-limited-c11-cext (setup.py): started
Running command python setup.py bdist_wheel
CC env var: 'gcc -pthread'
CFLAGS env var: <missing>
extra_compile_args: ['-Werror', '-Wcast-qual', '-Werror=declaration-after-statement', '-DMODULE_NAME=_test_limited_c11_cext', '-std=c11', '-DPy_LIMITED_API=0x30e00a0']
running bdist_wheel
running build
running build_ext
building '_test_limited_c11_cext' extension
creating build
creating build/temp.linux-x86_64-cpython-314
gcc -pthread -fno-strict-overflow -fstack-protector-strong -Wtrampolines -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 -Wsign-compare -DNDEBUG -g -O3 -Wall -fPIC -I/home/buildbot/buildarea/3.x.cstratak-RHEL8-x86_64.lto/build/build/test_python_2370939æ/tempcwd/env/include -I/home/buildbot/buildarea/3.x.cstratak-RHEL8-x86_64.lto/build/Include -I/home/buildbot/buildarea/3.x.cstratak-RHEL8-x86_64.lto/build -c extension.c -o build/temp.linux-x86_64-cpython-314/extension.o -Werror -Wcast-qual -Werror=declaration-after-statement -DMODULE_NAME=_test_limited_c11_cext -std=c11 -DPy_LIMITED_API=0x30e00a0
In file included from /usr/include/assert.h:35,
from /home/buildbot/buildarea/3.x.cstratak-RHEL8-x86_64.lto/build/Include/Python.h:19,
from extension.c:7:
/usr/include/features.h:393:5: error: #warning _FORTIFY_SOURCE > 2 is treated like 2 on this platform [-Werror=cpp]
# warning _FORTIFY_SOURCE > 2 is treated like 2 on this platform
^~~~~~~
cc1: all warnings being treated as errors
error: command '/usr/bin/gcc' failed with exit code 1
error: subprocess-exited-with-errorBASECFLAGS are baked into sysconfig and reused by tools like setuptools.
Consider adding extra flags to CFLAGS_NODIST, so they only affect CPython, not third-party extensions built for it. Those might have conflicting requirements.
nohlson
commented
1 month ago Consider adding extra flags to CFLAGS_NODIST, so they only affect CPython, not third-party extensions built for it. Those might have conflicting requirements.
I agree with limiting the scope of these options to just CPython. After testing with buildbots we can make this change as suggested by @corona10 here
encukou
commented
1 month ago I was redirected here: when adding configure options, I think it would be appropriate to have a wider discussion on Discourse rather than just on GitHub issues.
As I'm not following this effort very closely, I feel lost in the discussion and decisions :(
Practically, I think
sethmlarson
commented
1 month ago Apologies @encukou, agreed that these should get discussed to make sure they're the best method of handling this. The new options got added as a way to opt-in to performance-affecting compilation options or opt-out of options that aren't supported on your platform. See: https://github.com/python/cpython/issues/121996
I'll work with @nohlson to create this topic and issues for documenting them properly.
nohlson
commented
1 month ago I started a thread to have more discussion related to this topic: https://discuss.python.org/t/new-default-compiler-options-for-safety/60057/2
Attn: @encukou @sethmlarson
Feature or enhancement
Proposal:
At a recent meeting of OpenSSF's Memory Safety SIG, I became aware of the C/C++ hardening guide they are putting together.
At a high-level, they recommend compiling with the following flags:
(
-shareddoesn't really make sense as a global CFLAG, so I removed it.)When compiling on most x86 architectures (amd64, i386 and x32), add:
At @sethmlarson's urging, I compiled CPython on Linux/x86_64/gcc with these flags. From the complete build log, there are 3,084 warnings, but otherwise the result builds and passes all unit tests.
The warnings are of these types: (EDIT: Table updated to not double count the same line)
**Top warnings per file.**
| filename | count | | -- | --: | | ./Modules/binascii.c | 208 | | Objects/unicodeobject.c | 142 | | ./Include/internal/pycore_runtime_init.h | 128 | | Parser/parser.c | 114 | | ./Modules/_decimal/libmpdec/mpdecimal.c | 94 | | ./Modules/posixmodule.c | 85 | | ./Modules/socketmodule.c | 76 | | ./Modules/_pickle.c | 75 | | Objects/longobject.c | 65 | | ./Modules/arraymodule.c | 49 | | **total** | 3,084 |I am not a security expert, so I don't know a good way to assess how many of these are potentially exploitable, and how many are harmless false positives. Some are probably un-resolvable (format-literal is pretty hard to avoid when wrapping
sprintf, for example).At a high level, I think the process to address these and make incremental progress maybe looks something like:
But this is just to start the discussion about how to move forward.
Has this already been discussed elsewhere?
No response given
Links to previous discussion of this feature:
No response
Linked PRs