Page MenuHomePhabricator

-fstack-clash-protection: Return an actual error when used on unsupported OS
ClosedPublic

Authored by sylvestre.ledru on Nov 27 2020, 12:51 PM.

Details

Summary

$ clang-12: error: -fstack-clash-protection is not supported on Windows or Mac OS X

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptNov 27 2020, 12:51 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
sylvestre.ledru requested review of this revision.Nov 27 2020, 12:51 PM
kristof.beyls added inline comments.
clang/include/clang/Basic/DiagnosticDriverKinds.td
276–277

There are more OSes than Linux, Windows or OSX.
Maybe it's somewhat better to say "-fstack-clash-protection is not supported on %0", with the targeted OS being fed in.
If that is not easily possible, maybe just say "-fstack-clash-protection is not supported on the targeted OS"?

clang/include/clang/Basic/DiagnosticDriverKinds.td
276–277

Maybe it's somewhat better to say "-fstack-clash-protection is not supported on %0", with the targeted OS being fed in.

I second that one.

clang/lib/Driver/ToolChains/Clang.cpp
3074–3076

In that case you should probably allow explicitly the different Linux flavors here

Fix the error message

sylvestre.ledru marked 3 inline comments as done.Nov 30 2020, 2:09 AM
emaste added a subscriber: emaste.Nov 30 2020, 6:24 AM

Can we add a test that the feature can be enabled on an OS other than Linux / Windows / Darwin?

extend the test (thanks serge)

Herald added a project: Restricted Project. · View Herald TranscriptNov 30 2020, 9:29 AM
rnk added a subscriber: rnk.Nov 30 2020, 1:32 PM

Windows has effectively always had stack clash protection: we've always emitted those little chkstk probe calls for stack frames larger than a page. Would it make more sense to ignore this flag on Windows, since it opts into always-on behavior? If so, this doesn't seem like the right place to ignore it.

I see your comment here, but I don't really understand it:
https://reviews.llvm.org/D92100#2422729
What goes wrong on Windows? Can it be made to just work instead? It should be simple.

How do things go wrong on Darwin? I was under the impression that this was implemented in LLVM as strictly inline code, no runtime support required.

emaste added a comment.Dec 1 2020, 8:45 AM

How do things go wrong on Darwin? I was under the impression that this was implemented in LLVM as strictly inline code, no runtime support required.

That is my impression as well (although it seems that an earlier version might have emitted calls to a stack probe routine?)

How do things go wrong on Darwin? I was under the impression that this was implemented in LLVM as strictly inline code, no runtime support required.

That is my impression as well (although it seems that an earlier version might have emitted calls to a stack probe routine?)

The original security advisory doesn't mention Darwin, but there's nothing specific to Darwin in the stack clash protection implementation, so I'm fine with allowing it for Darwin too.

lkail added a subscriber: lkail.Dec 14 2020, 2:16 AM

@sylvestre.ledru I: double checked and there's nothing in the original advisory against Darwin, but nothing that clearly states it's protected either (unlike Windows-based system). And there's also nothing specific to Darwin in the stack clash protection implementation, I think it's ok to only warn on Windows.

I think it's ok to only warn on Windows.

IMO that's sensible

Error only on Windows

LGTM, thanks!

This revision is now accepted and ready to land.Dec 22 2020, 2:47 AM
This revision was landed with ongoing or failed builds.Dec 22 2020, 3:06 AM
This revision was automatically updated to reflect the committed changes.

This seems to break all tests on all platforms: http://45.33.8.238/linux/35854/step_7.txt

As far as I can tell, RenderSCPOptions is called unconditinoally and the error message is emitted at its start, before any Args.hasFlag checks -- so it's emitted every time a windows triple is passed.

Also, rnk's comment above wasn't addressed as far as I can tell.

Thanks for the revert!