From ab23670745a3b10ee118bf1a78ea220ae0f040ec Mon Sep 17 00:00:00 2001 From: Deepak Majeti Date: Wed, 16 Nov 2016 16:54:18 -0500 Subject: [PATCH 1/5] ORC-110: [C++] getTimezoneByFilename causes a crash when called from multiple threads --- c++/src/Timezone.cc | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/c++/src/Timezone.cc b/c++/src/Timezone.cc index b342a6cc0b..20f9d12b79 100644 --- a/c++/src/Timezone.cc +++ b/c++/src/Timezone.cc @@ -22,6 +22,7 @@ #include #include #include +#include #include #include #include @@ -650,6 +651,7 @@ namespace orc { DIAGNOSTIC_IGNORE("-Wglobal-constructors") DIAGNOSTIC_IGNORE("-Wexit-time-destructors") #endif + pthread_mutex_t timezone_mutex; static std::map timezoneCache; DIAGNOSTIC_POP @@ -735,7 +737,10 @@ namespace orc { * Get the local timezone. */ const Timezone& getLocalTimezone() { - return getTimezoneByFilename(LOCAL_TIMEZONE); + pthread_mutex_lock(&timezone_mutex); + const Timezone& timezone = getTimezoneByFilename(LOCAL_TIMEZONE); + pthread_mutex_unlock(&timezone_mutex); + return timezone; } /** @@ -746,7 +751,10 @@ namespace orc { std::string filename(getTimezoneDirectory()); filename += "/"; filename += zone; - return getTimezoneByFilename(filename); + pthread_mutex_lock(&timezone_mutex); + const Timezone& timezone = getTimezoneByFilename(filename); + pthread_mutex_unlock(&timezone_mutex); + return timezone; } /** From 7fe2d131ce4f669bea1d5364fbecd4e1fb726330 Mon Sep 17 00:00:00 2001 From: Deepak Majeti Date: Thu, 17 Nov 2016 10:31:54 -0500 Subject: [PATCH 2/5] add static mutex --- c++/src/Timezone.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/c++/src/Timezone.cc b/c++/src/Timezone.cc index 20f9d12b79..d64cb5c96f 100644 --- a/c++/src/Timezone.cc +++ b/c++/src/Timezone.cc @@ -651,7 +651,7 @@ namespace orc { DIAGNOSTIC_IGNORE("-Wglobal-constructors") DIAGNOSTIC_IGNORE("-Wexit-time-destructors") #endif - pthread_mutex_t timezone_mutex; + static pthread_mutex_t timezone_mutex; static std::map timezoneCache; DIAGNOSTIC_POP From 2e9972fa2b8beefca4ec4562fd9815d21706b7b9 Mon Sep 17 00:00:00 2001 From: Deepak Majeti Date: Thu, 17 Nov 2016 23:35:48 -0500 Subject: [PATCH 3/5] move lock inside getTimezoneByFilename --- c++/src/Timezone.cc | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/c++/src/Timezone.cc b/c++/src/Timezone.cc index d64cb5c96f..19151face8 100644 --- a/c++/src/Timezone.cc +++ b/c++/src/Timezone.cc @@ -690,6 +690,8 @@ namespace orc { * Results are cached. */ const Timezone& getTimezoneByFilename(const std::string& filename) { + // ORC-110 + pthread_mutex_lock(&timezone_mutex); std::map::iterator itr = timezoneCache.find(filename); if (itr != timezoneCache.end()) { @@ -730,6 +732,7 @@ namespace orc { } Timezone* result = new TimezoneImpl(filename, buffer); timezoneCache[filename] = result; + pthread_mutex_unlock(&timezone_mutex); return *result; } @@ -737,10 +740,7 @@ namespace orc { * Get the local timezone. */ const Timezone& getLocalTimezone() { - pthread_mutex_lock(&timezone_mutex); - const Timezone& timezone = getTimezoneByFilename(LOCAL_TIMEZONE); - pthread_mutex_unlock(&timezone_mutex); - return timezone; + return getTimezoneByFilename(LOCAL_TIMEZONE); } /** @@ -751,10 +751,7 @@ namespace orc { std::string filename(getTimezoneDirectory()); filename += "/"; filename += zone; - pthread_mutex_lock(&timezone_mutex); - const Timezone& timezone = getTimezoneByFilename(filename); - pthread_mutex_unlock(&timezone_mutex); - return timezone; + return getTimezoneByFilename(filename); } /** From edecaf3d314c2925764f4226ee717b744d3b9997 Mon Sep 17 00:00:00 2001 From: Deepak Majeti Date: Fri, 18 Nov 2016 07:51:44 -0500 Subject: [PATCH 4/5] Add unlocks before throws --- c++/src/Timezone.cc | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/c++/src/Timezone.cc b/c++/src/Timezone.cc index 19151face8..147b683ca5 100644 --- a/c++/src/Timezone.cc +++ b/c++/src/Timezone.cc @@ -695,23 +695,27 @@ namespace orc { std::map::iterator itr = timezoneCache.find(filename); if (itr != timezoneCache.end()) { + pthread_mutex_unlock(&timezone_mutex); return *(itr->second); } int in = open(filename.c_str(), O_RDONLY); if (in == -1) { std::stringstream buffer; buffer << "failed to open " << filename << " - " << strerror(errno); + pthread_mutex_unlock(&timezone_mutex); throw TimezoneError(buffer.str()); } struct stat fileInfo; if (fstat(in, &fileInfo) == -1) { std::stringstream buffer; buffer << "failed to stat " << filename << " - " << strerror(errno); + pthread_mutex_unlock(&timezone_mutex); throw TimezoneError(buffer.str()); } if ((fileInfo.st_mode & S_IFMT) != S_IFREG) { std::stringstream buffer; buffer << "non-file in tzfile reader " << filename; + pthread_mutex_unlock(&timezone_mutex); throw TimezoneError(buffer.str()); } size_t size = static_cast(fileInfo.st_size); @@ -720,6 +724,7 @@ namespace orc { while (posn < size) { ssize_t ret = read(in, &buffer[posn], size - posn); if (ret == -1) { + pthread_mutex_unlock(&timezone_mutex); throw TimezoneError(std::string("Failure to read timezone file ") + filename + " - " + strerror(errno)); } @@ -728,6 +733,7 @@ namespace orc { if (close(in) == -1) { std::stringstream err; err << "failed to close " << filename << " - " << strerror(errno); + pthread_mutex_unlock(&timezone_mutex); throw TimezoneError(err.str()); } Timezone* result = new TimezoneImpl(filename, buffer); From 8b9252c3aef12a6538d83f1ce09dca5d86fbcf59 Mon Sep 17 00:00:00 2001 From: Deepak Majeti Date: Fri, 18 Nov 2016 11:58:05 -0500 Subject: [PATCH 5/5] Add RAII locks --- c++/src/Adaptor.hh.in | 30 ++++++++++++++++++++++++++++++ c++/src/CMakeLists.txt | 9 +++++++++ c++/src/Timezone.cc | 12 ++---------- 3 files changed, 41 insertions(+), 10 deletions(-) diff --git a/c++/src/Adaptor.hh.in b/c++/src/Adaptor.hh.in index c897e2d7e3..34306e8a80 100644 --- a/c++/src/Adaptor.hh.in +++ b/c++/src/Adaptor.hh.in @@ -26,6 +26,7 @@ #cmakedefine HAS_PRE_1970 #cmakedefine HAS_POST_2038 #cmakedefine HAS_STD_ISNAN +#cmakedefine HAS_STD_MUTEX #cmakedefine NEEDS_REDUNDANT_MOVE #include "orc/orc-config.hh" @@ -112,4 +113,33 @@ #include #endif +#ifndef HAS_STD_MUTEX + #include + namespace orc { + /** + * Lock guard for pthread_mutex_t object using RAII + * The Lock is automatically release when exiting current scope. + */ + class LockORC { + public: + explicit LockORC(pthread_mutex_t& mutex) : mutex_ref_(mutex) { + pthread_mutex_lock(&mutex_ref_); + } + ~LockORC() { pthread_mutex_unlock(&mutex_ref_); } + private: + // no default constructor + LockORC(); + // prohibit copying + LockORC(const LockORC&); + LockORC& operator=(const LockORC&); + + pthread_mutex_t& mutex_ref_; + }; + } + #define std::mutex pthread_mutex_t + #define std::lock_guard LockORC +#else + #include +#endif + #endif /* ADAPTER_HH */ diff --git a/c++/src/CMakeLists.txt b/c++/src/CMakeLists.txt index 9e7cfa6b05..a1f79191de 100644 --- a/c++/src/CMakeLists.txt +++ b/c++/src/CMakeLists.txt @@ -66,6 +66,15 @@ CHECK_CXX_SOURCE_COMPILES(" HAS_STD_ISNAN ) +CHECK_CXX_SOURCE_COMPILES(" + #include + int main(int, char *[]) { + std::mutex test_mutex; + std::lock_guard lock_mutex(test_mutex); + }" + HAS_STD_MUTEX +) + CHECK_CXX_SOURCE_COMPILES(" #include std::string func() { diff --git a/c++/src/Timezone.cc b/c++/src/Timezone.cc index 147b683ca5..73026d2c0f 100644 --- a/c++/src/Timezone.cc +++ b/c++/src/Timezone.cc @@ -22,7 +22,6 @@ #include #include #include -#include #include #include #include @@ -651,7 +650,7 @@ namespace orc { DIAGNOSTIC_IGNORE("-Wglobal-constructors") DIAGNOSTIC_IGNORE("-Wexit-time-destructors") #endif - static pthread_mutex_t timezone_mutex; + static std::mutex timezone_mutex; static std::map timezoneCache; DIAGNOSTIC_POP @@ -691,31 +690,27 @@ namespace orc { */ const Timezone& getTimezoneByFilename(const std::string& filename) { // ORC-110 - pthread_mutex_lock(&timezone_mutex); + std::lock_guard timezone_lock(timezone_mutex); std::map::iterator itr = timezoneCache.find(filename); if (itr != timezoneCache.end()) { - pthread_mutex_unlock(&timezone_mutex); return *(itr->second); } int in = open(filename.c_str(), O_RDONLY); if (in == -1) { std::stringstream buffer; buffer << "failed to open " << filename << " - " << strerror(errno); - pthread_mutex_unlock(&timezone_mutex); throw TimezoneError(buffer.str()); } struct stat fileInfo; if (fstat(in, &fileInfo) == -1) { std::stringstream buffer; buffer << "failed to stat " << filename << " - " << strerror(errno); - pthread_mutex_unlock(&timezone_mutex); throw TimezoneError(buffer.str()); } if ((fileInfo.st_mode & S_IFMT) != S_IFREG) { std::stringstream buffer; buffer << "non-file in tzfile reader " << filename; - pthread_mutex_unlock(&timezone_mutex); throw TimezoneError(buffer.str()); } size_t size = static_cast(fileInfo.st_size); @@ -724,7 +719,6 @@ namespace orc { while (posn < size) { ssize_t ret = read(in, &buffer[posn], size - posn); if (ret == -1) { - pthread_mutex_unlock(&timezone_mutex); throw TimezoneError(std::string("Failure to read timezone file ") + filename + " - " + strerror(errno)); } @@ -733,12 +727,10 @@ namespace orc { if (close(in) == -1) { std::stringstream err; err << "failed to close " << filename << " - " << strerror(errno); - pthread_mutex_unlock(&timezone_mutex); throw TimezoneError(err.str()); } Timezone* result = new TimezoneImpl(filename, buffer); timezoneCache[filename] = result; - pthread_mutex_unlock(&timezone_mutex); return *result; }