GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-112301: Make fortify source option check -Werror by nohlson · Pull Request #122141 · python/cpython

Conversation

@nohlson

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

Uh oh!

There was an error while loading. Please reload this page.

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)

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nohlson
nohlson requested review from corona10 and erlend-aasland as code owners July 22, 2024 21:42
@bedevere-app bedevere-app Bot mentioned this pull request Jul 22, 2024
Consider applying flags for warnings about potential security issues #112301
Closed
@bedevere-app bedevere-app Bot added the awaiting review label Jul 22, 2024
corona10
corona10 requested changes Jul 23, 2024
View reviewed changes

@corona10 corona10 left a comment

Copy link
Copy Markdown
Member

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.

Please rebase the PR.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-app bedevere-app Bot removed the awaiting review label Jul 23, 2024
@bedevere-app

bedevere-app Bot commented Jul 23, 2024

Copy link
Copy Markdown

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot added the awaiting changes label Jul 23, 2024
@corona10 corona10 added the skip news label Jul 23, 2024
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
nohlson added 2 commits July 23, 2024 03:25
@nohlson
Make fortify source option check -Werror
98c011b
@nohlson
Move fortify source to disable safety for testing
505016a
@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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
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

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@nohlson nohlson Jul 23, 2024

Copy link
Copy Markdown
Contributor Author

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.

Removed. Ready to test with buildbots

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
nohlson added 2 commits July 23, 2024 13:04
@nohlson
Remove empty if block in configure for testing
63f9f2a
@nohlson
Update configure for testing
ea8ded7
@nohlson

nohlson commented Jul 23, 2024

Copy link
Copy Markdown
Contributor Author

I have made the requested changes; please review again
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot added awaiting change review and removed awaiting changes labels Jul 23, 2024
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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

Uh oh!

There was an error while loading. Please reload this page.

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nohlson
Revert test config
46404d8
@nohlson

nohlson commented Jul 24, 2024

Copy link
Copy Markdown
Contributor Author

I have made the requested changes; please review again
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app

bedevere-app Bot commented Jul 24, 2024

Copy link
Copy Markdown

Thanks for making the requested changes!

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
corona10
corona10 approved these changes Jul 25, 2024
View reviewed changes

@corona10 corona10 left a comment

Copy link
Copy Markdown
Member

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

Let's update BASECFLAGS into CFLAGS_NODIST with separate PR.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
nohlson reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting change review labels Jul 25, 2024
@corona10
corona10 merged commit ca0f7c4 into python:main Jul 25, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label 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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@nohlson @corona10 @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.