This is an archive of the discontinued LLVM Phabricator instance.

Add NO_EXEC_STACK_DIRECTIVE to chkstk and chkstk2 sources
ClosedPublic

Authored by dim on Dec 24 2017, 11:53 AM.

Details

Summary

As reported in bug 35739, the arch-specific sources for the chkstk and
chkstk2 functions do not contain NO_EXEC_STACK_DIRECTIVE macros, and
therefore have an executable stack by default. This can cause warnings
from automated vulnerability scanners.

Add the directives to make these warnings go away. This is basically a
follow-up to rL273500.

Event Timeline

dim created this revision.Dec 24 2017, 11:53 AM
Herald added subscribers: Restricted Project, javed.absar. · View Herald TranscriptDec 24 2017, 11:53 AM
martell accepted this revision.Dec 24 2017, 12:30 PM
martell added a subscriber: martell.

LGTM

Also noting my comment on 35739

chkstk.S and chkstk2.S should only be included on WIN32
removing them from the (NOT MSVC) guards seems like the correct move.
This revision is now accepted and ready to land.Dec 24 2017, 12:30 PM
dim updated this revision to Diff 128116.Dec 24 2017, 12:36 PM

Do not build chkstk.S and chkstk2.S for non-Windows targets.

dim updated this revision to Diff 128117.Dec 24 2017, 12:37 PM

Oops, checked in wrong revision. This has both fixes:

  • Add NO_EXEC_STACK_DIRECTIVE, that can never hurt
  • Remove chkstk*.S from non-Windows targets
martell added a subscriber: mstorsjo.EditedDec 24 2017, 12:50 PM

hmm, looking at the #define for NO_EXEC_STACK_DIRECTIVE
once the two files have been removed from non windows targets they won't do anything anymore.

Do you have commit access, it looks good as is?

dim added a comment.Dec 24 2017, 12:59 PM

hmm looking at the #define for NO_EXEC_STACK_DIRECTIVE
once the two files have been removed from non windows targets they won't do anything anymore.

Ah yes, you're right, I didn't realize that. Those won't really be needed then.

Do you have commit access, it looks good as is?

Yes, I'll commit.

This revision was automatically updated to reflect the committed changes.

hmm, looking at the #define for NO_EXEC_STACK_DIRECTIVE
once the two files have been removed from non windows targets they won't do anything anymore.

Yes, it's not really needed for chkstk for windows. As for what the condition should be, MINGW or "WIN32 and NOT MSVC", I don't have much of an opinion. (The comment I got in the mail said something about that, but this comment seems to have been revised?)