Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
j1elo opened this issue Jul 6, 2017 · 13 comments
Open

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

j1elo opened this issue Jul 6, 2017 · 13 comments

Comments

@j1elo
Copy link

j1elo commented Jul 6, 2017

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:

  • Sources for shared libraries are compiled as Position Independent Code, with the option -fPIC.
  • Then, shared libraries are linked also with option -fPIC.
  • Sources for executables are compiled as Position Independent Executable, with the option -fPIE.
  • Finally, objects of executables are linked with the options -fPIE -pie.

Example sources:

Building code for PIEs is achieved by adding '-fPIE' when compiling and '-fPIE -pie' when linking.

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
Copy link
Contributor

alehaa commented Jul 14, 2017

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
Copy link
Author

j1elo commented Jul 14, 2017

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
Copy link

ghost commented Jul 14, 2017

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
Copy link
Contributor

alehaa commented Jul 14, 2017

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

@ghost
Copy link

ghost commented Jul 15, 2017

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
Copy link

Alexander-Shukaev commented Jul 15, 2017

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

@j1elo
Copy link
Author

j1elo commented Jul 15, 2017

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

  • POSITION_INDEPENDENT_CODE is a boolean property defined for each individual target defined in CMake (either static libraries, shared libraries, or executables, via the add_library or add_executable commands). It is the idiomatic way of handling PIE/PIC code in CMake, so the authors recommend using this property instead of adding -fPIE or -fPIC flags manually to the compilation flags.
  • Its value by default is ON for shared libraries, and OFF for static libraries and executables.
  • However, if the variable CMAKE_POSITION_INDEPENDENT_CODE is set somewhere in the project, then the property POSITION_INDEPENDENT_CODE will be initialized with that value, regardless of the defaults.

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:

  • If the target is a library, the flag -fPIC is added by CMake to the compilation and linker steps.
  • If the target is an executable, the flag -fPIE is added by CMake to the compilation and linker steps.

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
Copy link

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

@jprotze
Copy link
Contributor

jprotze commented Jul 21, 2017

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
Copy link

ghost commented Jul 21, 2017

@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
Copy link
Author

j1elo commented Jan 12, 2018

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
Copy link

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
Copy link

TheErk commented Nov 13, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants