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

gh-112301: Make fortify source option check -Werror - #122141

Merged
corona10 merged 5 commits into
python:mainfrom
nohlson:foritfy-source-compile-flag-check-werror
Jul 25, 2024
Merged

gh-112301: Make fortify source option check -Werror#122141
corona10 merged 5 commits into
python:mainfrom
nohlson:foritfy-source-compile-flag-check-werror

Conversation

@nohlson

@nohlson nohlson commented Jul 22, 2024 •
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Contributor

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)

@bedevere-app bedevere-app Bot mentioned this pull request Jul 22, 2024
Closed
corona10
corona10 requested changes Jul 23, 2024
View reviewed changes

@corona10 corona10 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rebase the PR.

@bedevere-app

bedevere-app Bot commented Jul 23, 2024

Copy link
Copy Markdown

@corona10

corona10 commented Jul 23, 2024

Copy link
Copy Markdown
Member

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
nohlson force-pushed the foritfy-source-compile-flag-check-werror branch from 5d075bc to 505016a Compare July 23, 2024 08:34
@nohlson

nohlson commented Jul 23, 2024

Copy link
Copy Markdown
Contributor Author

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

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

bedevere-bot commented Jul 23, 2024

Copy link
Copy Markdown

🤖 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.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 23, 2024
corona10
corona10 reviewed Jul 23, 2024
View reviewed changes
Comment thread configure.ac Outdated
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])

@corona10 corona10 Jul 23, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
corona10 reviewed Jul 23, 2024
View reviewed changes
Comment thread 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])])

@corona10 corona10 Jul 23, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@nohlson nohlson Jul 23, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed. Ready to test with buildbots

@nohlson

nohlson commented Jul 23, 2024

Copy link
Copy Markdown
Contributor Author

I have made the requested changes; please review again

@bedevere-app

bedevere-app Bot commented Jul 23, 2024

Copy link
Copy Markdown

Thanks for making the requested changes!

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

@bedevere-app
bedevere-app Bot requested a review from corona10 July 23, 2024 20:10
@corona10 corona10 added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 23, 2024
@bedevere-bot

bedevere-bot commented Jul 23, 2024

Copy link
Copy Markdown

🤖 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.

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

corona10 commented Jul 24, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

@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

nohlson commented Jul 24, 2024

Copy link
Copy Markdown
Contributor Author

I have made the requested changes; please review again

@bedevere-app

bedevere-app Bot commented Jul 24, 2024

Copy link
Copy Markdown

corona10
corona10 approved these changes Jul 25, 2024
View reviewed changes

@corona10 corona10 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Let's update BASECFLAGS into CFLAGS_NODIST with separate PR.

@corona10
corona10 merged commit ca0f7c4 into python:main Jul 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@corona10 corona10 corona10 approved these changes
@erlend-aasland erlend-aasland Awaiting requested review from erlend-aasland erlend-aasland is a code owner

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@nohlson @corona10 @bedevere-bot