-
-
Notifications
You must be signed in to change notification settings - Fork 715
BUG: Fix locale-dependent parsing in NRRD reader causing metadata corruption #5684
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
base: main
Are you sure you want to change the base?
Changes from 3 commits
8e037e5
622bcb6
461a8a7
24262e2
0fc849d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,111 @@ | ||
| /*========================================================================= | ||
| * | ||
| * Copyright NumFOCUS | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * https://www.apache.org/licenses/LICENSE-2.0.txt | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| * | ||
| *=========================================================================*/ | ||
| #ifndef itkNumericLocale_h | ||
| #define itkNumericLocale_h | ||
|
|
||
| #include "itkMacro.h" | ||
| #include "ITKCommonExport.h" | ||
|
|
||
| #include <locale.h> | ||
| #include <cstring> | ||
| #include <cstdlib> | ||
|
|
||
| // Platform-specific includes for thread-safe locale handling | ||
| #if defined(_WIN32) | ||
| # include <windows.h> | ||
| #elif defined(__APPLE__) || defined(__linux__) || defined(__unix__) | ||
| // POSIX systems provide newlocale/uselocale in locale.h (C11) or xlocale.h (older) | ||
| # if defined(__APPLE__) | ||
| # include <xlocale.h> | ||
| # elif defined(__GLIBC__) | ||
| // glibc provides locale_t in locale.h | ||
| # else | ||
| // Try xlocale.h for other systems, fall back to locale.h | ||
| # if __has_include(<xlocale.h>) | ||
| # include <xlocale.h> | ||
| # endif | ||
| # endif | ||
| #endif | ||
|
|
||
| namespace itk | ||
| { | ||
|
|
||
| /** \class NumericLocale | ||
| * \brief RAII class for thread-safe temporary setting of LC_NUMERIC locale to "C". | ||
| * | ||
| * This class provides a thread-safe mechanism to temporarily set the LC_NUMERIC | ||
| * locale to "C" for locale-independent parsing and formatting of floating-point | ||
| * numbers. The original locale is automatically restored when the object goes | ||
| * out of scope. | ||
| * | ||
| * This is particularly useful when parsing file formats that use dot as decimal | ||
| * separator (like NRRD, VTK, etc.) regardless of the system locale setting. | ||
| * | ||
| * Thread safety: | ||
| * - On POSIX systems (Linux, macOS, BSD): Uses thread-local locale via uselocale() | ||
| * - On Windows: Uses thread-local locale via _configthreadlocale() | ||
| * - Fallback: Uses global setlocale() with mutex protection (not fully thread-safe | ||
| * for concurrent I/O operations, but prevents corruption of locale state) | ||
| * | ||
| * Example usage: | ||
| * \code | ||
| * { | ||
| * NumericLocale cLocale; | ||
| * // Parse file with dot decimal separator | ||
| * double value = std::strtod("3.14159", nullptr); | ||
| * // Locale automatically restored here | ||
| * } | ||
| * \endcode | ||
| * | ||
| * \ingroup ITKCommon | ||
| */ | ||
| class ITKCommon_EXPORT NumericLocale | ||
| { | ||
| public: | ||
| /** Constructor: Saves current LC_NUMERIC locale and sets it to "C" */ | ||
| NumericLocale(); | ||
|
|
||
| /** Destructor: Restores the original LC_NUMERIC locale */ | ||
| ~NumericLocale(); | ||
|
|
||
| // Delete copy and move operations | ||
| NumericLocale(const NumericLocale &) = delete; | ||
| NumericLocale & | ||
| operator=(const NumericLocale &) = delete; | ||
| NumericLocale(NumericLocale &&) = delete; | ||
| NumericLocale & | ||
| operator=(NumericLocale &&) = delete; | ||
|
|
||
| private: | ||
| #if defined(_WIN32) | ||
| // Windows: store previous thread-specific locale setting | ||
| int m_PreviousThreadLocaleSetting{ -1 }; | ||
| char * m_SavedLocale{ nullptr }; | ||
| #elif defined(__APPLE__) || defined(__linux__) || defined(__unix__) | ||
| // POSIX: store previous thread-local locale | ||
| locale_t m_PreviousLocale{ nullptr }; | ||
| locale_t m_CLocale{ nullptr }; | ||
| #else | ||
| // Fallback: global locale (protected by mutex in implementation) | ||
| char * m_SavedLocale{ nullptr }; | ||
|
||
| #endif | ||
| }; | ||
|
|
||
| } // end namespace itk | ||
|
|
||
| #endif // itkNumericLocale_h | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,139 @@ | ||||||||||||||||||||||||
| /*========================================================================= | ||||||||||||||||||||||||
| * | ||||||||||||||||||||||||
| * Copyright NumFOCUS | ||||||||||||||||||||||||
| * | ||||||||||||||||||||||||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||||||||||||||||||||||||
| * you may not use this file except in compliance with the License. | ||||||||||||||||||||||||
| * You may obtain a copy of the License at | ||||||||||||||||||||||||
| * | ||||||||||||||||||||||||
| * https://www.apache.org/licenses/LICENSE-2.0.txt | ||||||||||||||||||||||||
| * | ||||||||||||||||||||||||
| * Unless required by applicable law or agreed to in writing, software | ||||||||||||||||||||||||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||||||||||||||||||||||||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||||||||||||||||||||||||
| * See the License for the specific language governing permissions and | ||||||||||||||||||||||||
| * limitations under the License. | ||||||||||||||||||||||||
| * | ||||||||||||||||||||||||
| *=========================================================================*/ | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| #include "itkNumericLocale.h" | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| #include <mutex> | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| namespace itk | ||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of assuming the thread-specific locale method are an available based on preprocessor defined add CMake try_compile to detect the method. Follow the existing pattern in Modules/Core/Common/CMakeLists.txt, add the test source code to the same CMake directory, and add a cmakedefine to itkConfigurePrivate.h.in. If neither method is available produce a CMake warning, thread-safe locales are not supported and that the locale will need to be managed and in the application if needed. |
||||||||||||||||||||||||
| { | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| #if defined(_WIN32) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Windows implementation using thread-specific locale | ||||||||||||||||||||||||
| NumericLocale::NumericLocale() | ||||||||||||||||||||||||
| { | ||||||||||||||||||||||||
| // Enable thread-specific locale for this thread | ||||||||||||||||||||||||
| m_PreviousThreadLocaleSetting = _configthreadlocale(_ENABLE_PER_THREAD_LOCALE); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Save current LC_NUMERIC locale | ||||||||||||||||||||||||
| const char * currentLocale = setlocale(LC_NUMERIC, nullptr); | ||||||||||||||||||||||||
| if (currentLocale) | ||||||||||||||||||||||||
| { | ||||||||||||||||||||||||
| m_SavedLocale = _strdup(currentLocale); | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
| // If _strdup fails (returns nullptr), m_SavedLocale remains nullptr | ||||||||||||||||||||||||
| // and the locale will not be restored in the destructor | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Set to C locale for parsing | ||||||||||||||||||||||||
| setlocale(LC_NUMERIC, "C"); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| NumericLocale::~NumericLocale() | ||||||||||||||||||||||||
| { | ||||||||||||||||||||||||
| // Restore original locale | ||||||||||||||||||||||||
| if (m_SavedLocale) | ||||||||||||||||||||||||
| { | ||||||||||||||||||||||||
| setlocale(LC_NUMERIC, m_SavedLocale); | ||||||||||||||||||||||||
| free(m_SavedLocale); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Restore previous thread-specific locale setting | ||||||||||||||||||||||||
| if (m_PreviousThreadLocaleSetting != -1) | ||||||||||||||||||||||||
| { | ||||||||||||||||||||||||
| _configthreadlocale(m_PreviousThreadLocaleSetting); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| #elif defined(__APPLE__) || defined(__linux__) || defined(__unix__) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // POSIX implementation using thread-local locale (uselocale/newlocale) | ||||||||||||||||||||||||
| NumericLocale::NumericLocale() | ||||||||||||||||||||||||
| { | ||||||||||||||||||||||||
| // Create a new C locale | ||||||||||||||||||||||||
| // If newlocale fails (returns nullptr), m_CLocale remains nullptr | ||||||||||||||||||||||||
| // and the locale will not be changed - this is a safe fallback | ||||||||||||||||||||||||
| m_CLocale = newlocale(LC_NUMERIC_MASK, "C", nullptr); | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| if (m_CLocale) | ||||||||||||||||||||||||
| { | ||||||||||||||||||||||||
| // Set the C locale for this thread and save the previous locale | ||||||||||||||||||||||||
| // uselocale returns the previous locale, which may be LC_GLOBAL_LOCALE | ||||||||||||||||||||||||
| m_PreviousLocale = uselocale(m_CLocale); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| NumericLocale::~NumericLocale() | ||||||||||||||||||||||||
| { | ||||||||||||||||||||||||
| // If we created and installed a C locale for this thread, | ||||||||||||||||||||||||
| // restore the previous locale and free the C locale. | ||||||||||||||||||||||||
| // Always restore if m_CLocale is set, as m_PreviousLocale may be | ||||||||||||||||||||||||
| // LC_GLOBAL_LOCALE which is a valid non-null value. | ||||||||||||||||||||||||
| if (m_CLocale) | ||||||||||||||||||||||||
| { | ||||||||||||||||||||||||
| uselocale(m_PreviousLocale); | ||||||||||||||||||||||||
| freelocale(m_CLocale); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| #else | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Fallback implementation using global locale with mutex protection | ||||||||||||||||||||||||
| // This is not ideal but provides some protection against concurrent access | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
| namespace | ||||||||||||||||||||||||
| { | ||||||||||||||||||||||||
| std::mutex localeMutex; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| NumericLocale::NumericLocale() | ||||||||||||||||||||||||
| { | ||||||||||||||||||||||||
| // Lock mutex to protect global locale state | ||||||||||||||||||||||||
| // Using direct lock/unlock instead of lock_guard to maintain compatibility | ||||||||||||||||||||||||
| // with the RAII pattern where unlock happens in destructor | ||||||||||||||||||||||||
| localeMutex.lock(); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Save current LC_NUMERIC locale | ||||||||||||||||||||||||
| const char * currentLocale = setlocale(LC_NUMERIC, nullptr); | ||||||||||||||||||||||||
| if (currentLocale) | ||||||||||||||||||||||||
| { | ||||||||||||||||||||||||
| m_SavedLocale = strdup(currentLocale); | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
| m_SavedLocale = strdup(currentLocale); | |
| char * duplicatedLocale = strdup(currentLocale); | |
| if (duplicatedLocale != nullptr) | |
| { | |
| m_SavedLocale = duplicatedLocale; | |
| } | |
| // If duplication fails, m_SavedLocale remains null and the locale | |
| // will not be restored in the destructor; the "C" locale stays active. |
Outdated
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mutex is locked in the constructor but will remain locked if setlocale() on line 113 throws an exception. This creates a potential deadlock scenario. Consider using std::lock_guard or std::unique_lock for RAII-based mutex management to ensure the mutex is always unlocked even if an exception occurs.
| setlocale(LC_NUMERIC, "C"); | |
| try | |
| { | |
| setlocale(LC_NUMERIC, "C"); | |
| } | |
| catch (...) | |
| { | |
| // Ensure mutex is unlocked if setlocale throws, to avoid deadlock | |
| localeMutex.unlock(); | |
| throw; | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Including <windows.h> is unnecessary for this implementation. The _configthreadlocale, _strdup, and setlocale functions are provided by the C runtime library headers (<locale.h>, <stdlib.h>), not by windows.h. Including windows.h adds significant compilation overhead and can cause namespace pollution. Consider removing this include unless there's a specific reason for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove all system dependent include in the header. Add a forward declaration for a structure for a pointer to implementation or private implementation.