Hide extension symbols by default on Unix platforms

On platforms using libraries and executables in ELF format (e.g., Linux), exported symbols can collide. If a symbol (e.g., a function) is already available, loading a module with a function that has the exact same name will cause the newly loaded library to use the already existing function instead of the one in the new module. Unfortunately, GCC and Clang exports all symbols by default. To avoid symbol collisions, this change compiles the extension with `-fvisibility=hidden` on Unix platforms, which means a symbol isn't exported unless explicitly specified. Note that symbols are not exported by default on Windows. For GCC and Clang, a symbol can be exported by annotating a function or variable with `__attribute__ ((visibility ("default")))`. This is done transparently using our custom `TS_FUNCTION_INFO_V1` macro. Note that functions exported with that macro might still collide with existing symbols so they should have proper name prefixes that make collisions unlikely. A test for conflicting symbols runs in Debug configuration. When compiled in Debug configuration, code with an identically named function is added to both the loader and then versioned extension, although the functions return different outputs. The functions are called from both the loader and the versioned extension and the output should contain different module names if no conflict. The test fails on Linux if not compiled with -fvisibility=hidden.
timescale · Aug 24, 2018 · 027b7b2 · 027b7b2
1 parent 6a3abe5
commit 027b7b2
Show file tree

Hide file tree

Showing 13 changed files with 174 additions and 29 deletions.
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -1,4 +1,5 @@
 cmake_minimum_required(VERSION 3.4)
+include(CheckCCompilerFlag)
 
 configure_file("version.config" "version.config" COPYONLY)
 file(READ version.config VERSION_CONFIG)
@@ -19,6 +20,12 @@ else ()
   set(PROJECT_VERSION_MOD ${VERSION})
 endif ()
 
+# Set project name, version, and language. Language needs to be set for compiler checks
+project(timescaledb VERSION ${VERSION} LANGUAGES C)
+
+message(STATUS "TimescaleDB version ${PROJECT_VERSION_MOD}. Can be updated from version ${UPDATE_FROM_VERSION}")
+message(STATUS "Build type is ${CMAKE_BUILD_TYPE}")
+
 set(PROJECT_INSTALL_METHOD source CACHE STRING "Specify what install platform this binary
 is built for")
 
@@ -27,12 +34,32 @@ if (NOT CMAKE_BUILD_TYPE)
   set(CMAKE_BUILD_TYPE Release CACHE STRING "Choose the type of build, options are: None Debug Release RelWithDebInfo MinSizeRel" FORCE)
 endif ()
 
+set(SUPPORTED_COMPILERS "GNU" "Clang" "AppleClang" "MSVC")
+
+# Check for a supported compiler
+if (NOT CMAKE_C_COMPILER_ID IN_LIST SUPPORTED_COMPILERS)
+   message(FATAL_ERROR "Unsupported compiler ${CMAKE_C_COMPILER_ID}. Supported compilers are: ${SUPPORTED_COMPILERS}")
+endif ()
+
+# Check for supported platforms and compiler flags
 if (WIN32)
   if (NOT CMAKE_CONFIGURATION_TYPES)
     # Default to only include Release builds so MSBuild.exe 'just works'
     set(CMAKE_CONFIGURATION_TYPES Release CACHE STRING "Semicolon separated list of supported configuration types, only supports Debug, Release, MinSizeRel, and RelWithDebInfo, anything else will be ignored." FORCE)
   endif ()
-endif (WIN32)
+elseif (UNIX)
+  # On UNIX, the compiler needs to support -fvisibility=hidden to hide symbols by default
+  check_c_compiler_flag(-fvisibility=hidden CC_SUPPORTS_VISIBILITY_HIDDEN)
+
+  if (NOT CC_SUPPORTS_VISIBILITY_HIDDEN)
+    message(FATAL_ERROR "The compiler ${CMAKE_C_COMPILER_ID} does not support -fvisibility=hidden")
+  endif (NOT CC_SUPPORTS_VISIBILITY_HIDDEN)
+else ()
+  message(FATAL_ERROR "Unsupported platform")
+endif ()
+
+message(STATUS "Using compiler ${CMAKE_C_COMPILER_ID}")
+
 if (ENABLE_CODECOVERAGE)
   message(STATUS "Running code coverage")
 
@@ -57,11 +84,6 @@ if (ENABLE_CODECOVERAGE)
 
 endif (ENABLE_CODECOVERAGE)
 
-project(timescaledb VERSION ${VERSION} LANGUAGES C)
-
-message(STATUS "TimescaleDB version ${PROJECT_VERSION_MOD}. Can be updated from version ${UPDATE_FROM_VERSION}")
-message(STATUS "Build type is ${CMAKE_BUILD_TYPE}")
-
 # Search paths for Postgres binaries
 if (WIN32)
   find_path(PG_PATH

diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
@@ -3,7 +3,7 @@ set(CMAKE_C_FLAGS_DEBUG "-DUSE_ASSERT_CHECKING=1")
 if (UNIX)
   set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} -L${PG_LIBDIR}")
   set(CMAKE_MODULE_LINKER_FLAGS "${CMAKE_MODULE_LINKER_FLAGS} -L${PG_LIBDIR}")
-  set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} ${PG_CFLAGS}")
+  set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} ${PG_CFLAGS} -fvisibility=hidden")
   set(CMAKE_CPP_FLAGS "${CMAKE_CPP_FLAGS} ${PG_CPPFLAGS}")
   set(CMAKE_C_FLAGS_DEBUG "${CMAKE_C_FLAGS_DEBUG} -g")
 endif (UNIX)
@@ -58,6 +58,7 @@ set(HEADERS
   errors.h
   event_trigger.h
   extension.h
+  export.h
   guc.h
   hypercube.h
   hypertable_cache.h
@@ -125,6 +126,11 @@ set(SOURCES
 configure_file(version.h.in version.h)
 set(GITCOMMIT_H ${CMAKE_CURRENT_BINARY_DIR}/gitcommit.h)
 
+# Add test source code in Debug builds
+if (CMAKE_BUILD_TYPE MATCHES Debug)
+  set(TEST_SOURCES ../test/src/symbol_conflict.c)
+endif (CMAKE_BUILD_TYPE MATCHES Debug)
+
 if (WIN32)
   add_custom_command(
     OUTPUT ${GITCOMMIT_H}
@@ -164,12 +170,18 @@ if (PGINDENT)
     DEPENDS typedefs.list)
 endif (PGINDENT)
 
-add_library(${PROJECT_NAME} MODULE ${SOURCES} ${HEADERS} ${GITCOMMIT_H})
+add_library(${PROJECT_NAME} MODULE ${SOURCES} ${TEST_SOURCES} ${HEADERS} ${GITCOMMIT_H})
 
 set_target_properties(${PROJECT_NAME} PROPERTIES
     OUTPUT_NAME ${PROJECT_NAME}-${PROJECT_VERSION_MOD}
     PREFIX "")
 
+if (CMAKE_BUILD_TYPE MATCHES Debug)
+  # This define generates extension-specific code for symbol conflict testing
+  target_compile_definitions(${PROJECT_NAME} PRIVATE MODULE_NAME=${PROJECT_NAME})
+endif (CMAKE_BUILD_TYPE MATCHES Debug)
+
+
 install(
   TARGETS ${PROJECT_NAME}
   DESTINATION ${PG_PKGLIBDIR})

diff --git a/src/compat.h b/src/compat.h
@@ -1,6 +1,8 @@
 #ifndef TIMESCALEDB_COMPAT_H
 #define TIMESCALEDB_COMPAT_H
 
+#include "export.h"
+
 #define is_supported_pg_version_96(version) ((version >= 90603) && (version < 100000))
 #define is_supported_pg_version_10(version) ((version >= 100002) && (version < 110000))
 #define is_supported_pg_version(version) (is_supported_pg_version_96(version) || is_supported_pg_version_10(version))
@@ -56,8 +58,4 @@
 
 #endif							/* PG_VERSION_NUM */
 
-#define TS_FUNCTION_INFO_V1(fn) \
-	PGDLLEXPORT Datum fn(PG_FUNCTION_ARGS); \
-	PG_FUNCTION_INFO_V1(fn)
-
 #endif							/* TIMESCALEDB_COMPAT_H */
diff --git a/src/export.h b/src/export.h
@@ -0,0 +1,35 @@
+#ifndef TIMESCALEDB_EXPORT_H
+#define TIMESCALEDB_EXPORT_H
+
+#include <postgres.h>
+
+/* Definitions for symbol exports */
+
+#define TS_CAT(x,y) x ## y
+#define TS_EMPTY(x) (TS_CAT(x, 87628) == 87628)
+
+#if defined(_WIN32) && !defined(WIN32)
+#define WIN32
+#endif
+
+#if !defined(WIN32) && !defined(__CYGWIN__)
+#if __GNUC__ >= 4
+#if defined(PGDLLEXPORT)
+#if TS_EMPTY(PGDLLEXPORT)
+/* PGDLLEXPORT is defined but empty. We can safely undef it. */
+#undef PGDLLEXPORT
+#else
+#error "PGDLLEXPORT is already defined"
+#endif
+#endif							/* defined(PGDLLEXPORT) */
+#define PGDLLEXPORT __attribute__ ((visibility ("default")))
+#else
+#error "Unsupported GNUC version"
+#endif							/* __GNUC__ */
+#endif
+
+#define TS_FUNCTION_INFO_V1(fn) \
+	PGDLLEXPORT Datum fn(PG_FUNCTION_ARGS); \
+	PG_FUNCTION_INFO_V1(fn)
+
+#endif							/* TIMESCALEDB_EXPORT_H */
diff --git a/src/init.c b/src/init.c
@@ -9,6 +9,7 @@
 #include "guc.h"
 #include "catalog.h"
 #include "version.h"
+#include "compat.h"
 
 #ifdef PG_MODULE_MAGIC
 PG_MODULE_MAGIC;

diff --git a/src/loader/CMakeLists.txt b/src/loader/CMakeLists.txt
@@ -4,7 +4,19 @@ set(HEADERS
 set(SOURCES
   loader.c)
 
-add_library(${PROJECT_NAME}-loader MODULE ${SOURCES} ${HEADERS})
+# Include code for tests in Debug build
+if (CMAKE_BUILD_TYPE MATCHES Debug)
+  set(TEST_SOURCES ../../test/src/symbol_conflict.c)
+endif (CMAKE_BUILD_TYPE MATCHES Debug)
+
+add_library(${PROJECT_NAME}-loader MODULE ${SOURCES} ${TEST_SOURCES} ${HEADERS})
+
+if (CMAKE_BUILD_TYPE MATCHES Debug)
+  add_subdirectory(../../test/loader-mock/ "${CMAKE_CURRENT_BINARY_DIR}/mock")
+
+  # This define generates extension-specific code for symbol conflict testing
+  target_compile_definitions(${PROJECT_NAME}-loader PUBLIC MODULE_NAME=loader)
+endif (CMAKE_BUILD_TYPE MATCHES Debug)
 
 set_target_properties(${PROJECT_NAME}-loader PROPERTIES
     OUTPUT_NAME ${PROJECT_NAME}
@@ -13,7 +25,3 @@ set_target_properties(${PROJECT_NAME}-loader PROPERTIES
 install(
   TARGETS ${PROJECT_NAME}-loader
   DESTINATION ${PG_PKGLIBDIR})
-
-if(CMAKE_BUILD_TYPE MATCHES Debug)
-  add_subdirectory(../../test/loader-mock/ "${CMAKE_CURRENT_BINARY_DIR}/mock")
-endif(CMAKE_BUILD_TYPE MATCHES Debug)
diff --git a/src/loader/loader.c b/src/loader/loader.c
@@ -13,6 +13,7 @@
 #define EXTENSION_NAME "timescaledb"
 
 #include "../extension_utils.c"
+#include "../export.h"
 
 #define PG96 ((PG_VERSION_NUM >= 90600) && (PG_VERSION_NUM < 100000))
 #define PG10 ((PG_VERSION_NUM >= 100000) && (PG_VERSION_NUM < 110000))

diff --git a/test/expected/symbol_conflict.out b/test/expected/symbol_conflict.out
@@ -0,0 +1,28 @@
+\c single :ROLE_SUPERUSER
+-- Test for symbol conflicts between the loader module and the
+-- versioned extension module.
+-- This test fails on, e.g. Linux, unless compiled with -fvisibility=hidden
+CREATE OR REPLACE FUNCTION hello_loader() RETURNS TEXT
+AS 'timescaledb', 'loader_hello' LANGUAGE C IMMUTABLE PARALLEL SAFE STRICT;
+SELECT hello_loader();
+   hello_loader    
+-------------------
+ hello from loader
+(1 row)
+
+CREATE OR REPLACE FUNCTION hello_timescaledb() RETURNS TEXT
+AS :MODULE_PATHNAME, 'timescaledb_hello' LANGUAGE C IMMUTABLE PARALLEL SAFE STRICT;
+-- This calls an internal function with a conflicting name in the loader
+SELECT hello_loader();
+   hello_loader    
+-------------------
+ hello from loader
+(1 row)
+
+-- This calls the identically named internal function in the versioned extension
+SELECT hello_timescaledb();
+   hello_timescaledb    
+------------------------
+ hello from timescaledb
+(1 row)
+
diff --git a/test/loader-mock/src/CMakeLists.txt b/test/loader-mock/src/CMakeLists.txt
@@ -1,10 +1,10 @@
 set(SOURCES
-  init.c 
-  ../../../src/extension.c
-  ../../../src/guc.c
-  )
+  init.c
+  ${PROJECT_SOURCE_DIR}/src/extension.c
+  ${PROJECT_SOURCE_DIR}/src/guc.c
+)
 
-include_directories(BEFORE ${CMAKE_CURRENT_SOURCE_DIR})
+include_directories(BEFORE ${CMAKE_CURRENT_SOURCE_DIR} ${PROJECT_SOURCE_DIR}/src)
 
 add_library(${PROJECT_NAME}-mock-1 MODULE ${SOURCES} version.h)
 add_library(${PROJECT_NAME}-mock-2 MODULE ${SOURCES} version.h)
@@ -34,5 +34,3 @@ foreach(MOCK_VERSION mock-1 mock-2 mock-3 mock-4 mock-broken mock-5 mock-6)
     TARGETS ${PROJECT_NAME}-${MOCK_VERSION}
     DESTINATION ${PG_PKGLIBDIR} OPTIONAL)
 endforeach(MOCK_VERSION)
-
-
diff --git a/test/loader-mock/src/init.c b/test/loader-mock/src/init.c
@@ -9,13 +9,11 @@
 #include <nodes/print.h>
 #include <access/parallel.h>
 
+#include "export.h"
+
 #define STR_EXPAND(x) #x
 #define STR(x) STR_EXPAND(x)
 
-#define TS_FUNCTION_INFO_V1(fn) \
-	PGDLLEXPORT Datum fn(PG_FUNCTION_ARGS); \
-	PG_FUNCTION_INFO_V1(fn)
-
 #ifdef PG_MODULE_MAGIC
 PG_MODULE_MAGIC;
 #endif

diff --git a/test/sql/CMakeLists.txt b/test/sql/CMakeLists.txt
@@ -59,7 +59,7 @@ set(TEST_FILES
 
 IF(CMAKE_BUILD_TYPE MATCHES Debug)
   list(APPEND TEST_FILES
-    loader.sql)
+    loader.sql symbol_conflict.sql)
 ENDIF(CMAKE_BUILD_TYPE MATCHES Debug)
 
 IF (${PG_VERSION_MAJOR} GREATER "9")

diff --git a/test/sql/symbol_conflict.sql b/test/sql/symbol_conflict.sql
@@ -0,0 +1,17 @@
+\c single :ROLE_SUPERUSER
+
+-- Test for symbol conflicts between the loader module and the
+-- versioned extension module.
+-- This test fails on, e.g. Linux, unless compiled with -fvisibility=hidden
+CREATE OR REPLACE FUNCTION hello_loader() RETURNS TEXT
+AS 'timescaledb', 'loader_hello' LANGUAGE C IMMUTABLE PARALLEL SAFE STRICT;
+
+SELECT hello_loader();
+
+CREATE OR REPLACE FUNCTION hello_timescaledb() RETURNS TEXT
+AS :MODULE_PATHNAME, 'timescaledb_hello' LANGUAGE C IMMUTABLE PARALLEL SAFE STRICT;
+
+-- This calls an internal function with a conflicting name in the loader
+SELECT hello_loader();
+-- This calls the identically named internal function in the versioned extension
+SELECT hello_timescaledb();
diff --git a/test/src/symbol_conflict.c b/test/src/symbol_conflict.c
@@ -0,0 +1,27 @@
+#include <postgres.h>
+#include <utils/builtins.h>
+#include <fmgr.h>
+#include "../../src/compat.h"
+
+#define STR_EXPAND(x) #x
+#define STR(x) STR_EXPAND(x)
+
+#define FUNC_EXPAND(prefix, name) prefix##_##name
+#define FUNC(prefix, name) FUNC_EXPAND(prefix, name)
+
+/* Function with conflicting name when included in multiple modules */
+extern const char *test_symbol_conflict(void);
+
+const char *
+test_symbol_conflict(void)
+{
+	return "hello from " STR(MODULE_NAME);
+}
+
+TS_FUNCTION_INFO_V1(FUNC(MODULE_NAME, hello));
+
+Datum
+FUNC(MODULE_NAME, hello) (PG_FUNCTION_ARGS)
+{
+	PG_RETURN_TEXT_P(cstring_to_text(test_symbol_conflict()));
+}