Page MenuHomePhabricator

Use-after-return sanitizer binary metadata
ClosedPublic

Authored by dvyukov on Oct 17 2022, 6:32 AM.

Details

Summary

Currently per-function metadata consists of:
(start-pc, size, features)

This adds a new UAR feature and if it's set an additional element:
(start-pc, size, features, stack-args-size)

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

PTAL

llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp
252

What if Options.Covered==true?
Will it still emit some UAR metadata or will it emit something it shouldn't?

If Options.Covered==true && F.isVarArg(), we emit covered metadata with features=0. This looks WAI.

Should the F.isVarArg() check be done above in if (PerInstrFeatureMask || (Options.UAR && !F.isVarArg()) ? Then you wouldn't need the PerInstrFeatureMask && RequiresCovered change below and it could still just check RequiresCovered as before.

It's tricky.
I forgot why I structured the code this way, but I added a new test for all permutations of covered/atomics/uar and it shows the following breakage with your proposed change:

- CHECK-AU: ellipsis: features=1 stack_args=0
+ CHECK-AU: ellipsis: features=3 stack_args=0
llvm/test/Instrumentation/SanitizerBinaryMetadata/covered.cpp
1 ↗(On Diff #478569)

This is the new test.

melver accepted this revision.Nov 29 2022, 7:07 AM

LGTM

llvm/include/llvm/CodeGen/MachinePassRegistry.def
206

I called the LLVM IR pass just "sanmd-module".
So this could just be "machine-sanmd".

llvm/test/Instrumentation/SanitizerBinaryMetadata/common.h
54 ↗(On Diff #478569)

Could make this have its real signature, i.e. add version,start,end args. Then could also check in del that start,end matches, like above.

llvm/test/Instrumentation/SanitizerBinaryMetadata/covered.cpp
1 ↗(On Diff #478569)

Nice!

This revision is now accepted and ready to land.Nov 29 2022, 7:07 AM
melver added inline comments.Nov 29 2022, 7:12 AM
llvm/test/Instrumentation/SanitizerBinaryMetadata/uar.cpp
19 ↗(On Diff #478569)

Is this comment out of place?

dvyukov updated this revision to Diff 478609.Nov 29 2022, 8:08 AM
dvyukov marked 2 inline comments as done.

addressed comments

dvyukov marked an inline comment as done.Nov 29 2022, 8:08 AM
dvyukov added inline comments.
llvm/test/Instrumentation/SanitizerBinaryMetadata/uar.cpp
19 ↗(On Diff #478569)

If a function is completely empty, it won't be marked with UAR feature metadata.
This is super-primitive approximation of the actual analysis we should do (the function has addr-taken/escaped locals/arguments).
So I needed at least somebody for the function.

melver accepted this revision.Nov 29 2022, 8:12 AM
melver added inline comments.
llvm/test/Instrumentation/SanitizerBinaryMetadata/uar.cpp
19 ↗(On Diff #478569)

Ack

This revision was landed with ongoing or failed builds.Nov 29 2022, 8:37 AM
This revision was automatically updated to reflect the committed changes.
melver added inline comments.Nov 29 2022, 9:09 AM
llvm/lib/CodeGen/SanitizerBinaryMetadata.cpp
58

Probably should be getNumOperands()?

https://reviews.llvm.org/rGdbb11309665fdf7d411d25cc8f2e6a0c8f658143

-DLLVM_ENABLE_ASSERTIONS=On will catch it.

dvyukov reopened this revision.Nov 30 2022, 12:01 AM
This revision is now accepted and ready to land.Nov 30 2022, 12:01 AM
dvyukov updated this revision to Diff 478826.Nov 30 2022, 12:01 AM

fixed the debug build

This revision was landed with ongoing or failed builds.Nov 30 2022, 12:14 AM
This revision was automatically updated to reflect the committed changes.
dvyukov added a subscriber: kazu.Nov 30 2022, 12:15 AM

Kazu, thanks for taking care of the revert.

I've fixed the assert and tested with -DLLVM_ENABLE_ASSERTIONS=On.

dvyukov reopened this revision.Nov 30 2022, 12:39 AM
This revision is now accepted and ready to land.Nov 30 2022, 12:39 AM

FTR ./llvm/test/CodeGen/AMDGPU/llc-pipeline.ll also failed as it hardcodes all passes:
https://lab.llvm.org/buildbot/#/builders/123/builds/14397

FTR ./llvm/test/CodeGen/AMDGPU/llc-pipeline.ll also failed as it hardcodes all passes:
https://lab.llvm.org/buildbot/#/builders/123/builds/14397

Also these:

LLVM :: CodeGen/X86/O0-pipeline.ll
LLVM :: CodeGen/X86/opt-pipeline.ll

need to grep for something.

dvyukov updated this revision to Diff 478887.Nov 30 2022, 4:09 AM

moved *.cpp tests to clang/test/
updated tests that hardcode pipeline passes
fixed metadata update

PTAL

moved *.cpp tests to clang/test/
updated tests that hardcode pipeline passes
fixed metadata update

This did not work, it also changed all shared instances:

const auto *NewMD = MDB.createPCSections(
    {{Section.getString(), {Features, IRB.getInt32(Size)}}});
MD->replaceOperandWith(1, NewMD->getOperand(1));

so I changed it to:

F.setMetadata(LLVMContext::MD_pcsections,
              MDB.createPCSections(
                  {{Section.getString(), {Features, IRB.getInt32(Size)}}}));
melver requested changes to this revision.Nov 30 2022, 5:21 AM
This revision now requires changes to proceed.Nov 30 2022, 5:21 AM
dvyukov updated this revision to Diff 478912.Nov 30 2022, 5:28 AM

restrict tests to linux

melver accepted this revision.Nov 30 2022, 5:47 AM
This revision is now accepted and ready to land.Nov 30 2022, 5:47 AM
This revision was landed with ongoing or failed builds.Nov 30 2022, 5:50 AM
This revision was automatically updated to reflect the committed changes.

Hi, it looks like this change is causing Fuchsia's linux-x64 clang builders to fail: https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8796062278266465473/overview

Would you mind taking a look and fixing forward, or that ends up being difficult, reverting until this can be fixed?

aeubanks added inline comments.
clang/test/Instrumentation/SanitizerBinaryMetadata/common.h
1–3 ↗(On Diff #478922)

tests in clang/test cannot assume that standard headers are available, the tests should be hermetic. could you revert and reland with a proper test fix?

covered.cpp and uar.cpp seem to fail on ubuntu bots here.

dvyukov reopened this revision.Dec 1 2022, 8:34 AM
This revision is now accepted and ready to land.Dec 1 2022, 8:34 AM

@melver Re this failure:

******************** TEST 'Clang :: Instrumentation/SanitizerBinaryMetadata/uar.cpp' FAILED ********************
Script:
--
: 'RUN: at line 4';   /home/buildbot/as-worker-91/clang-with-lto-ubuntu/build/stage1/bin/clang --driver-mode=g++ /home/buildbot/as-worker-91/clang-with-lto-ubuntu/llvm-project/clang/test/Instrumentation/SanitizerBinaryMetadata/uar.cpp -o /home/buildbot/as-worker-91/clang-with-lto-ubuntu/build/stage1/tools/clang/test/Instrumentation/SanitizerBinaryMetadata/Output/uar.cpp.tmp -fexperimental-sanitize-metadata=covered,uar && /home/buildbot/as-worker-91/clang-with-lto-ubuntu/build/stage1/tools/clang/test/Instrumentation/SanitizerBinaryMetadata/Output/uar.cpp.tmp | /home/buildbot/as-worker-91/clang-with-lto-ubuntu/build/stage1/bin/FileCheck /home/buildbot/as-worker-91/clang-with-lto-ubuntu/llvm-project/clang/test/Instrumentation/SanitizerBinaryMetadata/uar.cpp
--
Exit Code: 1
Command Output (stderr):
--
/usr/local/bin/ld: sanmd_covered has both ordered [`sanmd_covered[_Z7consumeIjET_RPKcS2_]' in /tmp/lit-tmp-2jisr97l/uar-d5d8d4.o] and unordered [`sanmd_covered[__dummy_sanmd_covered]' in /tmp/lit-tmp-2jisr97l/uar-d5d8d4.o] sections
/usr/local/bin/ld: final link failed: Bad value
clang-16: error: linker command failed with exit code 1 (use -v to see invocation)

https://lab.llvm.org/buildbot/#/builders/124/builds/5759/steps/7/logs/stdio

This looks like a latent issue which is just exposed by the test.

As far as I understand this happens because this dummy variable somehow ends up in an "unordered" section, while other "real" metadata objects end up in "ordered" sections:

void SanitizerBinaryMetadata::createZeroSizedObjectInSection(
    Type *Ty, StringRef SectionSuffix) {
  auto *DummyInit = ConstantAggregateZero::get(ArrayType::get(Ty, 0));
  auto *DummyEntry = new GlobalVariable(Mod, DummyInit->getType(), true,
                                        GlobalVariable::ExternalLinkage,
                                        DummyInit, "__dummy_" + SectionSuffix);
  DummyEntry->setSection(getSectionName(SectionSuffix));
  DummyEntry->setVisibility(GlobalValue::HiddenVisibility);
  if (TargetTriple.supportsCOMDAT())
    DummyEntry->setComdat(Mod.getOrInsertComdat(DummyEntry->getName()));
  // Make sure the section isn't discarded by gc-sections.
  appendToUsed(Mod, DummyEntry);
}

I don't see any method on GlobalVariable to set the section to "ordered".

The only idea I have is to print dummy object in AsmPrinter the same way we print all other metadata objects (then I assume it will have the same properties as other objects).

Do you have any other ideas on how to dead with this?

Re this failure:

project/clang/test/Instrumentation/SanitizerBinaryMetadata/covered.cpp
--
Exit Code: 2

Command Output (stderr):
--
covered.cpp.tmp: /b/s/w/ir/x/w/llvm-llvm-project/clang/test/Instrumentation/SanitizerBinaryMetadata/common.h:22: T consume(const char *&, const char *) [T = unsigned int]: Assertion `pos <= end' failed.
FileCheck error: '<stdin>' is empty.
FileCheck command line:  /b/s/w/ir/x/w/staging/llvm_build/bin/FileCheck -check-prefix=CHECK-A /b/s/w/ir/x/w/llvm-llvm-project/clang/test/Instrumentation/SanitizerBinaryMetadata/covered.cpp

https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket/8796062278266465473/+/u/clang/test/stdout

Not sure why it run on fuchsia at all and the strange failure mode.
But I hope this will go away when we move tests to compiler-rt.

melver added a comment.Dec 1 2022, 9:35 AM

@melver Re this failure:

******************** TEST 'Clang :: Instrumentation/SanitizerBinaryMetadata/uar.cpp' FAILED ********************
Script:
--
: 'RUN: at line 4';   /home/buildbot/as-worker-91/clang-with-lto-ubuntu/build/stage1/bin/clang --driver-mode=g++ /home/buildbot/as-worker-91/clang-with-lto-ubuntu/llvm-project/clang/test/Instrumentation/SanitizerBinaryMetadata/uar.cpp -o /home/buildbot/as-worker-91/clang-with-lto-ubuntu/build/stage1/tools/clang/test/Instrumentation/SanitizerBinaryMetadata/Output/uar.cpp.tmp -fexperimental-sanitize-metadata=covered,uar && /home/buildbot/as-worker-91/clang-with-lto-ubuntu/build/stage1/tools/clang/test/Instrumentation/SanitizerBinaryMetadata/Output/uar.cpp.tmp | /home/buildbot/as-worker-91/clang-with-lto-ubuntu/build/stage1/bin/FileCheck /home/buildbot/as-worker-91/clang-with-lto-ubuntu/llvm-project/clang/test/Instrumentation/SanitizerBinaryMetadata/uar.cpp
--
Exit Code: 1
Command Output (stderr):
--
/usr/local/bin/ld: sanmd_covered has both ordered [`sanmd_covered[_Z7consumeIjET_RPKcS2_]' in /tmp/lit-tmp-2jisr97l/uar-d5d8d4.o] and unordered [`sanmd_covered[__dummy_sanmd_covered]' in /tmp/lit-tmp-2jisr97l/uar-d5d8d4.o] sections
/usr/local/bin/ld: final link failed: Bad value
clang-16: error: linker command failed with exit code 1 (use -v to see invocation)

https://lab.llvm.org/buildbot/#/builders/124/builds/5759/steps/7/logs/stdio

This looks like a latent issue which is just exposed by the test.

As far as I understand this happens because this dummy variable somehow ends up in an "unordered" section, while other "real" metadata objects end up in "ordered" sections:

void SanitizerBinaryMetadata::createZeroSizedObjectInSection(
    Type *Ty, StringRef SectionSuffix) {
  auto *DummyInit = ConstantAggregateZero::get(ArrayType::get(Ty, 0));
  auto *DummyEntry = new GlobalVariable(Mod, DummyInit->getType(), true,
                                        GlobalVariable::ExternalLinkage,
                                        DummyInit, "__dummy_" + SectionSuffix);
  DummyEntry->setSection(getSectionName(SectionSuffix));
  DummyEntry->setVisibility(GlobalValue::HiddenVisibility);
  if (TargetTriple.supportsCOMDAT())
    DummyEntry->setComdat(Mod.getOrInsertComdat(DummyEntry->getName()));
  // Make sure the section isn't discarded by gc-sections.
  appendToUsed(Mod, DummyEntry);
}

I don't see any method on GlobalVariable to set the section to "ordered".

The only idea I have is to print dummy object in AsmPrinter the same way we print all other metadata objects (then I assume it will have the same properties as other objects).

Do you have any other ideas on how to dead with this?

It's strange we've not encountered this elsewhere - this must be some special (old?) linker. I'll investigate...

melver added a comment.EditedDec 2 2022, 2:58 AM

Do you have any other ideas on how to dead with this?

It's strange we've not encountered this elsewhere - this must be some special (old?) linker. I'll investigate...

GNU ld <= 2.35 does not support mixed SHF_LINK_ORDER and non-SHF_LINK_ORDER sections.

This is about the relative order of the section w.r.t. other sections it references (we link it to the function section, see MCObjectFileInfo::getPCSection) when the linker merges the sections. From what I can find, we need to retain SHF_LINK_ORDER to support gc-sections properly.

This is a good summary: https://maskray.me/blog/2021-01-31-metadata-sections-comdat-and-shf-link-order
@MaskRay

I'm going to try and see how I can add the SHF_LINK_ORDER attribute to the section output for the dummy variable.

Do you have any other ideas on how to dead with this?

It's strange we've not encountered this elsewhere - this must be some special (old?) linker. I'll investigate...

GNU ld <= 2.35 does not support mixed SHF_LINK_ORDER and non-SHF_LINK_ORDER sections.

This is about the relative order of the section w.r.t. other sections it references (we link it to the function section, see MCObjectFileInfo::getPCSection) when the linker merges the sections. From what I can find, we need to retain SHF_LINK_ORDER to support gc-sections properly.

This is a good summary: https://maskray.me/blog/2021-01-31-metadata-sections-comdat-and-shf-link-order
@MaskRay

I'm going to try and see how I can add the SHF_LINK_ORDER attribute to the section output for the dummy variable.

This should fix it: https://reviews.llvm.org/D139224 - at least whatever I was able to reproduce with the linker version I found (2.31) seems to work.

SanitizerBinaryMetadata::createZeroSizedObjectInSection creates __dummy_*, but why is it needed? (There is no comment.)

melver added a comment.EditedDec 4 2022, 12:36 AM

SanitizerBinaryMetadata::createZeroSizedObjectInSection creates __dummy_*, but why is it needed? (There is no comment.)

// Create a 0-sized object in a section, so that the section is not discarded
// if all inputs have been discarded.

We need the __begin and __end markers even if there ends up being no binary metadata.

SanitizerBinaryMetadata::createZeroSizedObjectInSection creates __dummy_*, but why is it needed? (There is no comment.)

// Create a 0-sized object in a section, so that the section is not discarded
// if all inputs have been discarded.

We need the __begin and __end markers even if there ends up being no binary metadata.

Thanks! I saw the comment associated with createZeroSizedObjectInSection but wasn't so sure if it was only for section based garbage collection.
If that's the case, D139276 ExternalWeakLinkage would be a more size efficient fix.

dvyukov updated this revision to Diff 480051.Dec 5 2022, 4:35 AM

Moved tests to compiler-rt and rebased to HEAD.

Herald added a project: Restricted Project. · View Herald TranscriptDec 5 2022, 4:35 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
melver accepted this revision.Dec 5 2022, 5:19 AM
This revision was landed with ongoing or failed builds.Dec 5 2022, 5:40 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Dec 5 2022, 6:02 AM

This seems to break tests: http://45.33.8.238/linux/93224/step_12.txt

Can you take a look?

This seems to break tests: http://45.33.8.238/linux/93224/step_12.txt

Can you take a look?

Looking.

Fix for:

-- Supported architectures for crt: aarch64
CMake Error at compiler-rt/cmake/config-ix.cmake:244 (message):
  Unsupported architecture: x86_64
Call Stack (most recent call first):
  compiler-rt/cmake/config-ix.cmake:280 (get_target_flags_for_arch)
  compiler-rt/test/metadata/CMakeLists.txt:7 (get_test_cc_for_arch)
-- Configuring incomplete, errors occurred!

https://reviews.llvm.org/D139323

This seems to break tests: http://45.33.8.238/linux/93224/step_12.txt

Can you take a look?

Sent a fix: https://reviews.llvm.org/D139325