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

[mpividori]

Summary

This fix a bug, when calling InternalGetProcAddress() for an executable that doesn't export any symbol. So the table is empty.
If we don't check for this condition, the program fails with Error 0xc0000142.

[rnk]

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.

[rnk]

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

[zturner]

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]

Done.

[mpividori]

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.

[etienneb]

good catch
+1 for a test to catch this.

[mpividori]

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]

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]

@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]

lgtm

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

[kubamracek]

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/?

[mpividori]

@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]

In D28502#657267, @mpividori wrote:

@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]

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

[rnk]

Thanks, this is simpler.

[zturner]

lgtm

compiler-rt/trunk/lib/interception/interception_win.cc

if (!module || dos_stub->e_magic != IMAGE_DOS_SIGNATURE || // "MZ"
       headers->Signature != IMAGE_NT_SIGNATURE ||           // "PE\0\0"
       headers->FileHeader.SizeOfOptionalHeader <
           sizeof(IMAGE_OPTIONAL_HEADER)) {
     return 0;
   }
 
   IMAGE_DATA_DIRECTORY *export_directory =
       &headers->OptionalHeader.DataDirectory[IMAGE_DIRECTORY_ENTRY_EXPORT];
+  if (export_directory->Size == 0)
+    return 0;
   RVAPtr<IMAGE_EXPORT_DIRECTORY> exports(module,
                                          export_directory->VirtualAddress);
   RVAPtr<DWORD> functions(module, exports->AddressOfFunctions);
   RVAPtr<DWORD> names(module, exports->AddressOfNames);
   RVAPtr<WORD> ordinals(module, exports->AddressOfNameOrdinals);
 
   for (DWORD i = 0; i < exports->NumberOfNames; i++) {
     RVAPtr<char> name(module, names[i]);

compiler-rt/trunk/lib/interception/tests/interception_win_test.cc

#endif
   EXPECT_FALSE(TestFunctionPatching(kUnpatchableCode1, override, prefix));
   EXPECT_TRUE(TestFunctionPatching(kUnpatchableCode2, override, prefix));
   EXPECT_FALSE(TestFunctionPatching(kUnpatchableCode3, override, prefix));
   EXPECT_FALSE(TestFunctionPatching(kUnpatchableCode4, override, prefix));
   EXPECT_FALSE(TestFunctionPatching(kUnpatchableCode5, override, prefix));
   EXPECT_FALSE(TestFunctionPatching(kUnpatchableCode6, override, prefix));
 }
 
+TEST(Interception, EmptyExportTable) {
+  // We try to get a pointer to a function from an executable that doesn't
+  // export any symbol (empty export table).
+  uptr FunPtr = InternalGetProcAddress((void *)GetModuleHandleA(0), "example");
+  EXPECT_EQ(0U, FunPtr);
+}
+
 }  // namespace __interception
 
 #endif  // SANITIZER_WINDOWS
 #endif  // #if !SANITIZER_DEBUG