-
-
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 2 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,131 @@ | ||||||||||||||||||||||||||||||
| /*========================================================================= | ||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||
| * 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); | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // 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 | ||||||||||||||||||||||||||||||
| m_CLocale = newlocale(LC_NUMERIC_MASK, "C", nullptr); | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| if (m_CLocale) | ||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||
| // Set the C locale for this thread and save the previous locale | ||||||||||||||||||||||||||||||
| m_PreviousLocale = uselocale(m_CLocale); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| NumericLocale::~NumericLocale() | ||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||
| // Restore the previous locale for this thread | ||||||||||||||||||||||||||||||
| if (m_PreviousLocale) | ||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||
| uselocale(m_PreviousLocale); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // Free the C locale | ||||||||||||||||||||||||||||||
| if (m_CLocale) | ||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
| // Restore the previous locale for this thread | |
| if (m_PreviousLocale) | |
| { | |
| uselocale(m_PreviousLocale); | |
| } | |
| // Free the C locale | |
| if (m_CLocale) | |
| { | |
| // If we created and installed a C locale for this thread, | |
| // restore the previous locale and free the C locale. | |
| if (m_CLocale) | |
| { | |
| uselocale(m_PreviousLocale); |
Outdated
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.
Change the fall back implementation to only produce an ITK warning I the locale does not match the expected location. Do not use a mutex. Do not change the locale..
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 strdup function can return nullptr on allocation failure, but this case is not checked. If allocation fails, m_SavedLocale will be nullptr, which is handled safely in the destructor. However, it would be more robust to handle this case explicitly or document the assumption that allocation failure is acceptable here (since the locale will default to "C" and not be restored).
| 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.