Page MenuHomePhabricator

X86: Don't emit zero-byte functions on Windows
ClosedPublic

Authored by hans on Apr 20 2017, 5:23 PM.

Details

Summary

Empty functions can lead to duplicate entries in the Guard CF Function Table of a binary due to multiple functions sharing the same RVA, causing the kernel to refuse to load that binary.

We had a terrific bug due to this in Chromium.

Diff Detail

Event Timeline

hans created this revision.Apr 20 2017, 5:23 PM
MatzeB added a subscriber: MatzeB.Apr 20 2017, 5:48 PM

Sounds like a good story :)

  • We have various MachineInstr that do not result in actual code getting created (DEBUG_INFO, Labels, etc.) this wouldn't catch them.
  • There is a very similar situation handled in AsmPrinter.cpp already around line 1049:
// If the function is empty and the object file uses .subsections_via_symbols,
  // then we need to emit *something* to the function body to prevent the
  // labels from collapsing together.  Just emit a noop.

maybe you can do some refactoring there to do the COFF empty function handling as well?

mkuper edited edge metadata.Apr 20 2017, 5:51 PM

What does MSVC do for this? Is MSVC's behavior predicated on /guard:cf? Is it ok to generate empty functions without /guard:cf? Crash on calling an empty function seems like somewhat non-intuitive behavior.
Also, what about thumbv7-windows?

hans added a comment.Apr 21 2017, 10:19 AM

Sounds like a good story :)

  • We have various MachineInstr that do not result in actual code getting created (DEBUG_INFO, Labels, etc.) this wouldn't catch them.
  • There is a very similar situation handled in AsmPrinter.cpp already around line 1049: ` If the function is empty and the object file uses .subsections_via_symbols, then we need to emit *something* to the function body to prevent the // labels from collapsing together. Just emit a noop. ` maybe you can do some refactoring there to do the COFF empty function handling as well?

Nice! I'll try that.

What does MSVC do for this? Is MSVC's behavior predicated on /guard:cf? Is it ok to generate empty functions without /guard:cf? Crash on calling an empty function seems like somewhat non-intuitive behavior.
Also, what about thumbv7-windows?

I haven't been able to lure MSVC into emitting a zero-byte function. For example, for

__declspec(naked) void f() {}

they emit an int3.

We can't put this behind the /guard:cf flag (in fact, clang-cl doesn't even support it right now), because the problem happens when the linker is invoked with /guard:cf, it builds the guard table even if the compiled code isn't instrumented with control-flow checks.

I don't think crashing on calling a zero-byte function is non-intuitive; the function has no ret instruction, so anything could happen. Since Matthias pointed out we already have a function that inserts nops in similar situations, I'm happy to use that instead.

I don't know if CFG is a thing on thumbv7-windows, but if we're doing it in the asm printer now, it's easy to just enable for all windows targets.

hans updated this revision to Diff 96167.Apr 21 2017, 10:21 AM

Use the already existing functionality in AsmPrinter to insert a nop.

hans added inline comments.Apr 21 2017, 10:22 AM
lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1056

I checked, and there are no in-tree targets that "opt out" in this way.
It would be awkward, because getNoopForMachoTarget() is also used for other things, e.g. ARMAsmPrinter::EmitSled.

MatzeB added inline comments.Apr 21 2017, 10:51 AM
lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1053–1054

This whole code feels a bit out of place, as we have macho and windows specific handling as part of the generic AsmPrinter now. Should at least have a FIXME or better a new interface to hide this behind some API. So you can query MCAsmInfo or MCTargetStreamer (not sure what is best with my limited MC experience) whether empty functions should be avoided.

1056

You can just rename as part of this commit instead of adding a FIXME. getNoopForEmptyFunction() maybe?

hans added inline comments.Apr 21 2017, 1:45 PM
lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1053–1054

I'll add a FIXME for now.

1056

I'll rename to just getNoop(). It seems the main use is for empty functions, but ARMAsmPrinter::EmitSled() also uses it for XRay instrumentation so it seems it really is just used for getting nop instructions.

hans updated this revision to Diff 96222.Apr 21 2017, 1:46 PM

Renamed getNoopForMachoTarget() to getNoop() and added FIXME.

Please take another look.

MatzeB accepted this revision.Apr 21 2017, 1:49 PM

LGTM

lib/Target/ARM/ARMInstrInfo.h
28 ↗(On Diff #96222)

Could have just removed the "getNoop -" part as we discourage that style nowadays.

This revision is now accepted and ready to land.Apr 21 2017, 1:49 PM
hans added inline comments.Apr 21 2017, 2:10 PM
lib/Target/ARM/ARMInstrInfo.h
28 ↗(On Diff #96222)

Good point. Will do that when committing.

This revision was automatically updated to reflect the committed changes.