Page MenuHomePhabricator

[compiler-rt] [Interception] Properly check for export table's size before referring to its elements.
ClosedPublic

Authored by mpividori on Jan 9 2017, 4:52 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

mpividori updated this revision to Diff 83743.Jan 9 2017, 4:52 PM
mpividori retitled this revision from to [compiler-rt] [Interception] Properly check for export table's size before referring to its elements..
mpividori updated this object.
mpividori added reviewers: zturner, rnk.
mpividori set the repository for this revision to rL LLVM.
mpividori added a subscriber: llvm-commits.
rnk accepted this revision.Jan 9 2017, 4:55 PM
rnk edited edge metadata.

Feel free to commit if you like the suggestion.

lib/interception/interception_win.cc
881 ↗(On Diff #83743)

It's probably more readable to do == 0 since Size is a number, not a pointer.

This revision is now accepted and ready to land.Jan 9 2017, 4:55 PM
rnk requested changes to this revision.Jan 9 2017, 4:56 PM
rnk edited edge metadata.

Actually, if you hit this in normal usage, can you construct a test case for this?

This revision now requires changes to proceed.Jan 9 2017, 4:56 PM
zturner accepted this revision.Jan 9 2017, 4:56 PM
zturner edited edge metadata.
zturner added inline comments.
lib/interception/interception_win.cc
881 ↗(On Diff #83743)

The parentheses aren't necessary, and can you compare it explicitly to 0 instead of using an implicit bool conversion?

Otherwise, lgtm.

mpividori updated this revision to Diff 83746.Jan 9 2017, 5:00 PM
mpividori edited edge metadata.
mpividori removed rL LLVM as the repository for this revision.

Done.

In D28502#640799, @rnk wrote:

Actually, if you hit this in normal usage, can you construct a test case for this?

Ok. I will add a test.

good catch
+1 for a test to catch this.

Ok. Some questions about the implementation of tests:

  • I couldn't find any information about the files: lit.common.cfg . Are the same than lib.cfg? should I use "--config-prefix=lit.common"? I see some directories include "lit.cfg" and other include "lit.common.cfg". The documentation doesn't mention "lit.common" files.
  • How should I run compiler-rt tests? After reading lit's documentation, I suppose it should work if I run: " python llvm-lit.py path-to-test-folder " But it always fails.

For example, running: "python bin/llvm-lit.py ../llvm/projects/compiler-rt/test" , I get:

llvm-lit.py: ...llvm\utils\lit\lit\discovery.py:113: warning: unable to find test suite for '../llvm/projects/compiler-rt/test'
llvm-lit.py: ...llvm\utils\lit\lit\discovery.py:224: warning: input '../llvm/projects/compiler-rt/test' contained no tests

I would appreciate your comments, I think lit's documentation is outdated.
Thanks,

rnk added a comment.Jan 10 2017, 1:48 PM

Running the compiler-rt tests with llvm-lit.py is a real pain because we run many test configurations from the same build directory. Here is what I do to run a single asan test:

# from a build directory
$ ./bin/llvm-lit.py -v projects/compiler-rt/test/asan/X86_64WindowsConfig/TestCases/Windows/dll_null_deref.cc
-- Testing: 1 tests, 1 threads --
PASS: AddressSanitizer-x86_64-windows :: TestCases/Windows/dll_null_deref.cc (1 of 1)
Testing Time: 2.23s
  Expected Passes    : 1

Adjust "X86_64WindowsConfig" as appropriate for your target. You can also run X86_64WindowsDynamicConfig to run the tests with the dynamic CRT (/MD). Tab completion of individual test names doesn't work, because the tests are in the source directory, not the build directory.

mpividori updated this revision to Diff 83987.Jan 11 2017, 8:58 AM
mpividori edited edge metadata.
mpividori set the repository for this revision to rL LLVM.

@rnk Thanks for that information. Finally, I think I understood how tests works.
In this updated diff I add a regression test for Windows. It fails if we don't check the size of the export table.

rnk accepted this revision.Jan 11 2017, 1:21 PM
rnk edited edge metadata.

lgtm

The duplication to make a new lit test suite bothers me, but oh well.

This revision is now accepted and ready to land.Jan 11 2017, 1:21 PM
This revision was automatically updated to reflect the committed changes.
kubamracek reopened this revision.Jan 25 2017, 10:03 PM

This was reverted, so reopening. Were there bot failures?

Also, I might be late to the party, but I don't think adding a new test suite for this it a good idea. Lit tests are supposed to be "user code". Your test includes an internal header file and calls an internal function. Can you reproduce the issue in a "user code"-type test (how did you hit the issue in the first place)? And can you put it into an existing test suite, e.g. into test/asan/TestCases/Windows/?

This revision is now accepted and ready to land.Jan 25 2017, 10:03 PM
kubamracek requested changes to this revision.Jan 25 2017, 10:04 PM
This revision now requires changes to proceed.Jan 25 2017, 10:04 PM

@kubamracek Thanks for your comments. Yes, I had to revert it because of the cmake code for the test. I need to review it.
As this was a bug in the interception library, I think we should include it in the interception directory, instead of asan library.
I discovered this bug when updating sanitizer coverage for windows. In https://reviews.llvm.org/D29168 , when asan dll is initialized, I analyse the exported symbols in the main executable. It was failing when the main executable doesn't export any symbol. Then I realized there was a bug in the interception library.
As I considered it a "regression test", and I read in the documentation: http://llvm.org/docs/TestingGuide.html that regression tests should go in the repository/test folder, I decided to include the test there.

@kubamracek Thanks for your comments. Yes, I had to revert it because of the cmake code for the test. I need to review it.
As this was a bug in the interception library, I think we should include it in the interception directory, instead of asan library.
I discovered this bug when updating sanitizer coverage for windows. In https://reviews.llvm.org/D29168 , when asan dll is initialized, I analyse the exported symbols in the main executable. It was failing when the main executable doesn't export any symbol. Then I realized there was a bug in the interception library.
As I considered it a "regression test", and I read in the documentation: http://llvm.org/docs/TestingGuide.html that regression tests should go in the repository/test folder, I decided to include the test there.

The "interception" library isn't provided to user code directly. Either write a gtest-style unit test instead (in lib/interception/tests) or put the lit test into an existing sanitizer that is affected by the bug. Adding a lit test suite that links against internals is just wrong.

mpividori updated this revision to Diff 86245.Jan 29 2017, 11:45 PM
mpividori edited edge metadata.

@kubamracek @zturner
I updated the diff to use a gtest.

rnk added a comment.Jan 30 2017, 8:59 AM

Thanks, this is simpler.

This revision was automatically updated to reflect the committed changes.