[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

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

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

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]);

test/interception/CMakeLists.txt

 set(INTERCEPTION_TEST_DEPS ${SANITIZER_COMMON_LIT_TEST_DEPS})
 set(INTERCEPTION_TESTSUITES)
+set(INTERCEPTION_LIT_SOURCE_DIR ${CMAKE_CURRENT_SOURCE_DIR})
+
+foreach(arch ${SANITIZER_COMMON_SUPPORTED_ARCH})
+  set(INTERCEPTION_TEST_TARGET_ARCH ${arch})
+  set(CONFIG_NAME ${arch}-${OS_NAME})
+
+  get_test_cc_for_arch(${arch} INTERCEPTION_TEST_TARGET_CC
+    INTERCEPTION_TEST_TARGET_CFLAGS)
+
+  string(TOUPPER ${arch} ARCH_UPPER_CASE)
+  set(CONFIG_NAME ${ARCH_UPPER_CASE}${OS_NAME}Config)
+  set(INTERCEPTION_TEST_TARGET_LIB RTInterception.test.${arch})
+
+  get_property(INTERCEPTION_TEST_TARGET_LIB_DIR TARGET
+    ${INTERCEPTION_TEST_TARGET_LIB} PROPERTY ARCHIVE_OUTPUT_DIRECTORY)
+
+  configure_lit_site_cfg(
+    ${CMAKE_CURRENT_SOURCE_DIR}/lit.site.cfg.in
+    ${CMAKE_CURRENT_BINARY_DIR}/${CONFIG_NAME}/lit.site.cfg
+    )
+
+  list(APPEND INTERCEPTION_TESTSUITES
+    ${CMAKE_CURRENT_BINARY_DIR}/${CONFIG_NAME})
+  list(APPEND INTERCEPTION_TEST_DEPS RTInterception.test.${arch})
+endforeach()
 
 # Unit tests. There are currently no unit tests capable to running on Apple or
 # Android targets.
 if(COMPILER_RT_INCLUDE_TESTS AND NOT ANDROID AND NOT APPLE)
   configure_lit_site_cfg(
     ${CMAKE_CURRENT_SOURCE_DIR}/Unit/lit.site.cfg.in
     ${CMAKE_CURRENT_BINARY_DIR}/Unit/lit.site.cfg)
   list(APPEND INTERCEPTION_TESTSUITES ${CMAKE_CURRENT_BINARY_DIR}/Unit)
   list(APPEND INTERCEPTION_TEST_DEPS InterceptionUnitTests)
 endif()
 
 add_lit_testsuite(check-interception "Running the Interception tests"
   ${INTERCEPTION_TESTSUITES}
   DEPENDS ${INTERCEPTION_TEST_DEPS})
 set_target_properties(check-interception PROPERTIES FOLDER "Compiler-RT Misc")

test/interception/TestCases/Windows/empty_export_table.cc

+// RUN: %clang_cl %s %inter_lib -o %t
+// RUN: %run %t 2>&1 | FileCheck %s
+// CHECK: OK
+
+#include "interception/interception.h"
+#include <stdio.h>
+#include <windows.h>
+
+// We try to get a pointer to a function from an executable that doesn't export
+// any symbol (empty export table).
+int main() {
+  __sanitizer::uptr FunPtr = __interception::InternalGetProcAddress(
+      (void *)GetModuleHandleA(0), "exampleFun");
+  if (FunPtr == 0)
+    printf("OK");
+  return 0;
+}

test/interception/TestCases/Windows/lit.local.cfg

+def getRoot(config):
+  if not config.parent:
+    return config
+  return getRoot(config.parent)
+
+root = getRoot(config)
+
+if root.host_os not in ['Windows']:
+  config.unsupported = True

test/interception/lit.cfg

+# -*- Python -*-
+
+# Setup source root.
+config.test_source_root = os.path.join(os.path.dirname(__file__), "TestCases")
+
+# Setup config name.
+config.name = "Interception"
+
+# Add library sustitutions.
+if config.host_os == 'Windows':
+  libpath = os.path.join(config.target_lib_dir, config.target_lib + ".lib")
+  config.substitutions.append( ("%inter_lib", libpath))
+else:
+  libpath = os.path.join(config.target_lib_dir, "lib" + config.target_lib + ".a")
+  config.substitutions.append( ("%inter_lib", libpath))
+
+# Add clang substitutions.
+config.substitutions.append( ("%clang ", " ".join([config.clang,
+    config.target_cflags, "-I"+config.sources_dir, ""]) ))
+
+# Configure clang_cl for Windows.
+if config.host_os == 'Windows':
+  clang_cl_invocation = config.clang.replace("clang.exe","clang-cl.exe")
+  config.substitutions.append( ("%clang_cl ", " ".join([clang_cl_invocation,
+      config.target_cflags, "/I"+config.sources_dir, ""]) ))
+
+# Files to be considered for tests.
+config.suffixes = ['.c', '.cc', '.cpp']

test/interception/lit.site.cfg.in

+@LIT_SITE_CFG_IN_HEADER@
+
+# Tool-specific config options.
+config.target_cflags = "@INTERCEPTION_TEST_TARGET_CFLAGS@"
+config.clang = "@INTERCEPTION_TEST_TARGET_CC@"
+config.target_arch = "@INTERCEPTION_TEST_TARGET_ARCH@"
+config.target_lib = "@INTERCEPTION_TEST_TARGET_LIB@"
+config.target_lib_dir = "@INTERCEPTION_TEST_TARGET_LIB_DIR@"
+config.sources_dir = os.path.join("@COMPILER_RT_SOURCE_DIR@", "lib")
+
+# Load common config for all compiler-rt lit tests.
+lit_config.load_config(config, "@COMPILER_RT_BINARY_DIR@/test/lit.common.configured")
+
+# Load tool-specific config that would do the real work.
+lit_config.load_config(config, "@INTERCEPTION_LIT_SOURCE_DIR@/lit.cfg")