gh-112301: Make fortify source option check -Werror by nohlson · Pull Request #122141 · python/cpython

nohlson · 2024-07-22T21:42:12Z

Add autoconf option to AX_CHECK_COMPILE_FLAG in configure.ac to treat checking for -D_FORTIFY_SOURCE=3 compiler option availability with -Werror to match other options.

Suggested in previous PR for adding this option: #121520 (comment)

Issue: Consider applying flags for warnings about potential security issues #112301

corona10

Please rebase the PR.

bedevere-app · 2024-07-23T00:22:37Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

corona10 · 2024-07-23T01:04:28Z

If you want to test the option with build bot please move the option under the --disable-safety then we can run the options with buildbot and if the build is success let's move the option to the --enable-slower-safety .

We can add --enable-slower-safety to the buildbot options with separate PRs.

nohlson · 2024-07-23T08:35:53Z

@corona10 I have pushed a commit to put -D_FORTIFY_SOURCE=3 to --disable-safety so we can test with buildbots

bedevere-bot · 2024-07-23T08:44:09Z

🤖 New build scheduled with the buildbot fleet by @corona10 for commit 505016a 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

corona10 · 2024-07-23T08:44:57Z

configure.ac

 then
  AX_CHECK_COMPILE_FLAG([-fstack-protector-strong], [BASECFLAGS="$BASECFLAGS -fstack-protector-strong"], [AC_MSG_WARN([-fstack-protector-strong not supported])], [-Werror])
  AX_CHECK_COMPILE_FLAG([-Wtrampolines], [BASECFLAGS="$BASECFLAGS -Wtrampolines"], [AC_MSG_WARN([-Wtrampolines not supported])], [-Werror])
+  AX_CHECK_COMPILE_FLAG([-D_FORTIFY_SOURCE=3], [BASECFLAGS="$BASECFLAGS -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3"], [AC_MSG_WARN([-D_FORTIFY_SOURCE=3 not supported])], [-Werror])


cc @encukou

It will be moved to ./configure --enable-slower-safety after the buildbot is passed.
And maybe we can discuss about using CFLAGS_NODIST in this PR too.

corona10 · 2024-07-23T11:23:37Z

configure.ac


 if test "$enable_slower_safety" = "yes"
 then
-  AX_CHECK_COMPILE_FLAG([-D_FORTIFY_SOURCE=3], [BASECFLAGS="$BASECFLAGS -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3"], [AC_MSG_WARN([-D_FORTIFY_SOURCE=3 not supported])])


@nohlson you can remove whole code block for testing purpose.

Removed. Ready to test with buildbots

nohlson · 2024-07-23T20:10:44Z

I have made the requested changes; please review again

bedevere-app · 2024-07-23T20:10:49Z

Thanks for making the requested changes!

@corona10: please review the changes made to this pull request.

bedevere-bot · 2024-07-23T20:37:05Z

🤖 New build scheduled with the buildbot fleet by @corona10 for commit ea8ded7 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

corona10 · 2024-07-24T12:48:23Z

@nohlson

Refleaks from test_types does not related to this PR

Objects/object.c:1061:16: runtime error: call to function long_hash through pointer to incorrect function type 'long (*)(struct _object *)'
/buildbot/buildarea/pull_request.pablogsal-arch-x86_64.clang-ubsan/build/Objects/longobject.c:3618: note: long_hash defined here
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior Objects/object.c:1061:16 in
make: *** [Makefile:1691: Python/frozen_modules/getpath.h] Error

also does not related to this PR.

So the change itself looks fine.
Could you move the fortify source option to --enable-slower-safety?

nohlson · 2024-07-24T23:57:16Z

I have made the requested changes; please review again

bedevere-app · 2024-07-24T23:57:19Z

Thanks for making the requested changes!

@corona10: please review the changes made to this pull request.

corona10

lgtm

Let's update BASECFLAGS into CFLAGS_NODIST with separate PR.

nohlson requested review from corona10 and erlend-aasland as code owners July 22, 2024 21:42

bedevere-app bot mentioned this pull request Jul 22, 2024

Consider applying flags for warnings about potential security issues #112301

Open

bedevere-app bot added the awaiting review label Jul 22, 2024

corona10 requested changes Jul 23, 2024

View reviewed changes

bedevere-app bot removed the awaiting review label Jul 23, 2024

bedevere-app bot added the awaiting changes label Jul 23, 2024

corona10 added the skip news label Jul 23, 2024

nohlson added 2 commits July 23, 2024 03:25

Make fortify source option check -Werror

98c011b

Move fortify source to disable safety for testing

505016a

nohlson force-pushed the foritfy-source-compile-flag-check-werror branch from 5d075bc to 505016a Compare July 23, 2024 08:34

corona10 added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 23, 2024

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 23, 2024

corona10 reviewed Jul 23, 2024

View reviewed changes

nohlson added 2 commits July 23, 2024 13:04

Remove empty if block in configure for testing

63f9f2a

Update configure for testing

ea8ded7

bedevere-app bot added awaiting change review and removed awaiting changes labels Jul 23, 2024

bedevere-app bot requested a review from corona10 July 23, 2024 20:10

corona10 added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 23, 2024

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 23, 2024

Revert test config

46404d8

corona10 approved these changes Jul 25, 2024

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting change review labels Jul 25, 2024

corona10 merged commit ca0f7c4 into python:main Jul 25, 2024

bedevere-app bot removed the awaiting merge label Jul 25, 2024

Uh oh!

Conversation

nohlson commented Jul 22, 2024 • edited by bedevere-app bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

corona10 left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-app bot commented Jul 23, 2024

Uh oh!

corona10 commented Jul 23, 2024

Uh oh!

nohlson commented Jul 23, 2024

Uh oh!

bedevere-bot commented Jul 23, 2024

Uh oh!

corona10 Jul 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corona10 Jul 23, 2024

Choose a reason for hiding this comment

Uh oh!

nohlson Jul 23, 2024

Choose a reason for hiding this comment

Uh oh!

nohlson commented Jul 23, 2024

Uh oh!

bedevere-app bot commented Jul 23, 2024

Uh oh!

bedevere-bot commented Jul 23, 2024

Uh oh!

corona10 commented Jul 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nohlson commented Jul 24, 2024

Uh oh!

bedevere-app bot commented Jul 24, 2024

Uh oh!

corona10 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nohlson commented Jul 22, 2024 •

edited by bedevere-app bot

Loading

corona10 Jul 23, 2024 •

edited

Loading

corona10 commented Jul 24, 2024 •

edited

Loading