arsenm / sanitizers-cmake

CMake modules to help use sanitizers
Other
375 stars 65 forks source link

[Question] Shouldn't TSan add the flag "-pie"? #13

Open j1elo opened 7 years ago

j1elo commented 7 years ago

Seems that ThreadSanitizer would need that the source code is compiled and linked with position-independent object code. Source: https://github.com/google/sanitizers/wiki/ThreadSanitizerDevelopment

your/fresh/gcc test.c -fsanitize=thread -g -O1 -fPIE -pie

This is a summary of how to use those options:

Example sources:

CMake chooses the appropriate compilation flag when the option CMAKE_POSITION_INDEPENDENT_CODE is set to ON (either -fPIC or -fPIE depending on the type of target). However, it does not do the same for the linker flags when creating the final executable.

The project Apache Arrow acknowledges this and adds the "-pie" option to the linker step: https://github.com/apache/arrow/blob/master/cpp/cmake_modules/san-config.cmake#L75

set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -pie -fsanitize=thread")

Curiously enough, they don't add the option "-fPIE" to the linker flags, so their implementation may also be incomplete.

There seems to be a lot of confusion about this topic from around 2012 - 2014; I'm not sure of what was the conclusion of this topic but clearly some platforms such as Android seem to now enforce the usage of PIE executables. On the desktop, I don't know if the main Linux distributions have standardized on using PIE or not.

This is more an open call to discuss the issue rather than simply a request to add the "-pie" option to the compiler flags that get added by FindTSan.cmake.

alehaa commented 7 years ago

Sorry, but I don't get your point. As far as I understand you want to know whether-fPIE and -fpie flags have to be set, right? Clang sets them automatically when using the thread sanitizer:

Non-position-independent executables are not supported. Therefore, the fsanitize=thread flag will cause Clang to act as though the -fPIE flag had been supplied if compiling without -fPIC, and as though the -pie flag had been supplied if linking an executable.

j1elo commented 7 years ago

I see, it is then superfluous that the ThreadSanitizerCppManual shows this as an example call to Clang:

clang++ simple_race.cc -fsanitize=thread -fPIE -pie -g

On the other hand, the comment you have pointed out might be only applicable to Clang. I haven't been able to find any similar note on the documentation for GCC.

As I wrote earlier, ThreadSanitizerDevelopment also seems to imply that the compiler should be called with those flags, in their example call to GCC:

your/fresh/gcc test.c -fsanitize=thread -g -O1 -fPIE -pie

but at this point I would accept that their example commands include "-fPIE -pie" just for the sake of completeness and being more explicit.

It is then a matter of finding out if GCC does actually include the required flags when compiling a file with the -fsanitize=thread, or not. Do you have any insight into this?

ghost commented 7 years ago

I've tested a few things.

First of all, main.c:

#include <stdio.h>
#include <stdlib.h>
#include <pthread.h>

#define NUM_THREADS 2

/* create thread argument struct for thr_func() */
typedef struct _thread_data_t {
  int tid;
  double stuff;
} thread_data_t;

/* thread function */
void *thr_func(void *arg) {
  thread_data_t *data = (thread_data_t *)arg;

  printf("hello from thr_func, thread id: %d\n", data->tid);

  pthread_exit(NULL);
}

int main(int argc, char **argv) {
  pthread_t thr[NUM_THREADS];
  int i, rc;
  /* create a thread_data_t argument array */
  thread_data_t thr_data[NUM_THREADS];

  /* create threads */
  for (i = 0; i < NUM_THREADS; ++i) {
    thr_data[i].tid = i;
    if ((rc = pthread_create(&thr[i], NULL, thr_func, &thr_data[i]))) {
      fprintf(stderr, "error: pthread_create, rc: %d\n", rc);
      return EXIT_FAILURE;
    }
  }
  /* block until all threads complete */
  for (i = 0; i < NUM_THREADS; ++i) {
    pthread_join(thr[i], NULL);
  }

  return EXIT_SUCCESS;
}

CMakeLists.txt:

cmake_minimum_required(VERSION 3.3 FATAL_ERROR)
project(pthread_mininimal_example C)
list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_LIST_DIR}/cmake")
# set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -frecord-gcc-switches")
find_package(Sanitizers REQUIRED)
find_package(Threads REQUIRED)
add_executable(pthread_mininimal_example main.c)
add_sanitizers(pthread_mininimal_example)
target_link_libraries(pthread_mininimal_example Threads::Threads)

GCC has an option -frecord-gcc-switches that records the flags:

This switch causes the command line that was used to invoke the compiler to be recorded into the object file that is being created. This switch is only implemented on some targets and the exact format of the recording is target and binary file format dependent, but it usually takes the form of a section containing ASCII text.

Let's try it!

CMake configuration and generation:

 cmake -DCMAKE_C_COMPILER=/usr/bin/gcc -DCMAKE_BUILD_TYPE=Debug -DCMAKE_VERBOSE_MAKEFILE=ON -DSANITIZE_THREAD=ON ..
-- The C compiler identification is GNU 7.1.1
-- Check for working C compiler: /usr/bin/gcc
-- Check for working C compiler: /usr/bin/gcc -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- Try GNU ThreadSanitizer flag = [-g -fsanitize=thread]
-- Performing Test TSan_FLAG_DETECTED
-- Performing Test TSan_FLAG_DETECTED - Success
-- Looking for pthread.h
-- Looking for pthread.h - found
-- Looking for pthread_create
-- Looking for pthread_create - not found
-- Looking for pthread_create in pthreads
-- Looking for pthread_create in pthreads - not found
-- Looking for pthread_create in pthread
-- Looking for pthread_create in pthread - found
-- Found Threads: TRUE
-- Configuring done
-- Generating done
-- Build files have been written to: /home/l2y/Documents/code/open-source/pthread-min-example/build

Compilation:

 make V=1
/usr/bin/cmake -H/home/l2y/Documents/code/open-source/pthread-min-example -B/home/l2y/Documents/code/open-source/pthread-min-example/build --check-build-system CMakeFiles/Makefile.cmake 0
/usr/bin/cmake -E cmake_progress_start /home/l2y/Documents/code/open-source/pthread-min-example/build/CMakeFiles /home/l2y/Documents/code/open-source/pthread-min-example/build/CMakeFiles/progress.marks
make -f CMakeFiles/Makefile2 all
make[1]: Entering directory '/home/l2y/Documents/code/open-source/pthread-min-example/build'
make -f CMakeFiles/pthread_mininimal_example.dir/build.make CMakeFiles/pthread_mininimal_example.dir/depend
make[2]: Entering directory '/home/l2y/Documents/code/open-source/pthread-min-example/build'
cd /home/l2y/Documents/code/open-source/pthread-min-example/build && /usr/bin/cmake -E cmake_depends "Unix Makefiles" /home/l2y/Documents/code/open-source/pthread-min-example /home/l2y/Documents/code/open-source/pthread-min-example /home/l2y/Documents/code/open-source/pthread-min-example/build /home/l2y/Documents/code/open-source/pthread-min-example/build /home/l2y/Documents/code/open-source/pthread-min-example/build/CMakeFiles/pthread_mininimal_example.dir/DependInfo.cmake --color=
Scanning dependencies of target pthread_mininimal_example
make[2]: Leaving directory '/home/l2y/Documents/code/open-source/pthread-min-example/build'
make -f CMakeFiles/pthread_mininimal_example.dir/build.make CMakeFiles/pthread_mininimal_example.dir/build
make[2]: Entering directory '/home/l2y/Documents/code/open-source/pthread-min-example/build'
[ 50%] Building C object CMakeFiles/pthread_mininimal_example.dir/main.c.o
/usr/bin/gcc   -frecord-gcc-switches -g    -g -fsanitize=thread  -o CMakeFiles/pthread_mininimal_example.dir/main.c.o   -c /home/l2y/Documents/code/open-source/pthread-min-example/main.c
[100%] Linking C executable pthread_mininimal_example
/usr/bin/cmake -E cmake_link_script CMakeFiles/pthread_mininimal_example.dir/link.txt --verbose=1
/usr/bin/gcc  -frecord-gcc-switches -g  -rdynamic  -g -fsanitize=thread CMakeFiles/pthread_mininimal_example.dir/main.c.o  -o pthread_mininimal_example -lpthread
make[2]: Leaving directory '/home/l2y/Documents/code/open-source/pthread-min-example/build'
[100%] Built target pthread_mininimal_example
make[1]: Leaving directory '/home/l2y/Documents/code/open-source/pthread-min-example/build'
/usr/bin/cmake -E cmake_progress_start /home/l2y/Documents/code/open-source/pthread-min-example/build/CMakeFiles 0

Finally, flags:

 readelf -p .GCC.command.line pthread_mininimal_example

String dump of section '.GCC.command.line':
  [     0]  /home/l2y/Documents/code/open-source/pthread-min-example/main.c
  [    40]  -mtune=generic
  [    4f]  -march=x86-64
  [    5d]  -auxbase-strip CMakeFiles/pthread_mininimal_example.dir/main.c.o
  [    9e]  -g
  [    a1]  -frecord-gcc-switches
  [    b7]  -fsanitize=thread

So, no -fPIE -pie. Or there is, but it is provided through some other mechanism than just appending a flag to the whole thing. The binary successfully launches through ./pthread_mininimal_example and terminates.

I also tried the same, but with Clang. It doesn't recognize -frecord-gcc-switches and the whole generation fails. Ironically, without a notice of a problematic compiler option supplied before TSans' ones:

  cmake -DCMAKE_C_COMPILER=/usr/bin/clang -DCMAKE_BUILD_TYPE=Debug -DCMAKE_VERBOSE_MAKEFILE=ON -DSANITIZE_THREAD=ON ..
-- The C compiler identification is Clang 4.0.1
-- Check for working C compiler: /usr/bin/clang
-- Check for working C compiler: /usr/bin/clang -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- Try Clang ThreadSanitizer flag = [-g -fsanitize=thread]
-- Performing Test TSan_FLAG_DETECTED
-- Performing Test TSan_FLAG_DETECTED - Failed
CMake Warning at cmake/sanitize-helpers.cmake:143 (message):
  ThreadSanitizer is not available for Clang compiler.  Targets using this
  compiler will be compiled without ThreadSanitizer.
Call Stack (most recent call first):
  cmake/FindTSan.cmake:53 (sanitizer_check_compiler_flags)
  cmake/FindSanitizers.cmake:38 (find_package)
  CMakeLists.txt:5 (find_package)

-- Looking for pthread.h
-- Looking for pthread.h - not found
CMake Error at /usr/share/cmake-3.8/Modules/FindPackageHandleStandardArgs.cmake:137 (message):
  Could NOT find Threads (missing: Threads_FOUND)
Call Stack (most recent call first):
  /usr/share/cmake-3.8/Modules/FindPackageHandleStandardArgs.cmake:377 (_FPHSA_FAILURE_MESSAGE)
  /usr/share/cmake-3.8/Modules/FindThreads.cmake:212 (FIND_PACKAGE_HANDLE_STANDARD_ARGS)
  CMakeLists.txt:6 (find_package)

-- Configuring incomplete, errors occurred!
See also "/home/l2y/Documents/code/open-source/pthread-min-example/build/CMakeFiles/CMakeOutput.log".
See also "/home/l2y/Documents/code/open-source/pthread-min-example/build/CMakeFiles/CMakeError.log".

I didn't find any similar switch for Clang. If it adds the mentioned options, then it does this internally.

Commenting out set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -frecord-gcc-switches") generates and builds everything just like in case of GCC. The binary successfully launches and terminates.

I know little about this, but I thought some practical tests might be beneficial for you.

alehaa commented 7 years ago

Ok, so we should add -fPIE and -fpie to the flags to ensure ThreadSanitizer works with gcc and clang?

ghost commented 7 years ago

Today I found the hardening-check utility initially based on Debian's wiki and then updated by @Alexander-Shukaev: https://bitbucket.org/Alexander-Shukaev/hardening-check

Running it with the same testing setup as above with just CMAKE_C_COMPILER changed produces

...
Position Independent Executable (PIE): no (regular executable)
...

for both Clang and GCC.

Then I tried running it with -DCMAKE_POSITION_INDEPENDENT_CODE=ON, but for both compilers this only appends -fPIE to compile flags and nothing to link flags. So, the check still reports it's not a PIE.

Then I just manually appended them:


#...
set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -fPIE")
set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -fPIE -pie")
#...

And reconfiguring the project produces the expected result for both compilers:

...
Position Independent Executable (PIE): yes
...

So, to sum up.

  1. CMake has some functional support of building PIC shared libraries (see POSITION_INDEPENDENT_CODE property), but lacks full support for PIE (passing only C_FLAG). -fpic or -PIC were not tested with the libraries, so there's some space for further experiments, this is only about PIE.
  2. For PIE, setting CMAKE_<LANG>_FLAGS (if POSITION_INDEPENDENT_CODE property is set on the target, there should be no need, but nevertheless) and CMAKE_EXE_LINKER_FLAGS as above is the only solution as of CMake 3.8.2.
Alexander-Shukaev commented 7 years ago

Indeed, here is how I automated hardening within my CMake Helpers in a portable way:

j1elo commented 7 years ago

I had previously studied the behavior of POSITION_INDEPENDENT_CODE in CMake, and can comment on how it works:

The effect of setting POSITION_INDEPENDENT_CODE to ON for a CMake target (or setting CMAKE_POSITION_INDEPENDENT_CODE for the whole project), is the following:

However, CMake is lacking that it does not add the flag -pie to the linker step of executable targets!

In my projects, I have been needing to do this in the root CMakeFiles.txt:

# Use "-fPIC" / "-fPIE" for all targets by default, including static libs
set(CMAKE_POSITION_INDEPENDENT_CODE ON)

# CMake doesn't add "-pie" by default for executables (CMake issue #14983)
set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -pie")

There has been some discussion about this topic, and at some point they added the -pie flag when POSITION_INDEPENDENT_CODE is ON, but only for Android projects: https://gitlab.kitware.com/cmake/cmake/issues/16382

There is an open issue but it has been largely ignored so far:

Maybe it is the time to bring more attention to this issue in their bugtracker... I couldn't find any relevant discussion where they may have decided that adding -pie is not desirable.

Alexander-Shukaev commented 7 years ago

@j1elo, exactly what I've worked around in CMake Helpers.

jprotze commented 7 years ago

My experience working with ThreadSanitizer for some time matches with @light2yellow: recent gcc + clang compiler versions, at least on x86 architectures, don't need the pi* flags. The flags were only necessary in the early versions of the Sanitizers.

ghost commented 7 years ago

@j1elo Yes, indeed, my info was not fully correct - it adds -fPIE to the linker flags.

I also asked about POSITION_INDEPENDENT_CODE behaviour on CMake Users mailing list.

j1elo commented 6 years ago

I've just updated the CMake issue #14983 with a comprehensive description of how to use -fPIC / -fPIE.

@jprotze, are you sure that the Sanitizers don't need Position Independent Code and Position Independent Executable binaries any more? According to the ThreadSanitizer limitations description,

Non-position-independent executables are not supported. Therefore, the fsanitize=thread flag will cause Clang to act as though the -fPIE flag had been supplied if compiling without -fPIC, and as though the -pie flag had been supplied if linking an executable.

This could mean that Clang (and probably GCC too) is doing the job of actually adding those flags implicitly if you don't add them. Which is nice, but quoting a very well acclaimed best practice, "explicit is better than implicit".

craigscott-crascit commented 6 years ago

To provide an update on the CMake side of things, there's now a merge request for this being reviewed in CMake here:

https://gitlab.kitware.com/cmake/cmake/merge_requests/2465

TheErk commented 5 years ago

Small follow-up: MR https://gitlab.kitware.com/cmake/cmake/merge_requests/2465 has been recently merged and associated ticket https://gitlab.kitware.com/cmake/cmake/merge_requests/2465 closed.