-
-
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
Open
Copilot
wants to merge
5
commits into
main
Choose a base branch
from
copilot/fix-nrrd-reader-locale-issue
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+697
−0
Open
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
8e037e5
BUG: Fix locale-dependent parsing in NRRD reader and add test
Copilot 622bcb6
ENH: Add thread-safe NumericLocale utility class
Copilot 461a8a7
Address code review comments on NumericLocale
Copilot 24262e2
Refactor NumericLocale per code review feedback
Copilot 0fc849d
Add Google Test for NumericLocale class
Copilot File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
31 changes: 31 additions & 0 deletions
31
Modules/Core/Common/CMake/itkCheckHasConfigthreadlocale.cxx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| /*========================================================================= | ||
| * | ||
| * 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. | ||
| * | ||
| *=========================================================================*/ | ||
|
|
||
| // Check if _configthreadlocale is available (Windows thread-local locale) | ||
| #include <locale.h> | ||
|
|
||
| int | ||
| main() | ||
| { | ||
| int prev = _configthreadlocale(_ENABLE_PER_THREAD_LOCALE); | ||
| if (prev != -1) | ||
| { | ||
| _configthreadlocale(prev); | ||
| } | ||
| return 0; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| /*========================================================================= | ||
| * | ||
| * 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. | ||
| * | ||
| *=========================================================================*/ | ||
|
|
||
| // Check if newlocale and uselocale are available (POSIX thread-local locale) | ||
| #include <locale.h> | ||
|
|
||
| #if defined(__APPLE__) | ||
| # include <xlocale.h> | ||
| #endif | ||
|
|
||
| int | ||
| main() | ||
| { | ||
| locale_t loc = newlocale(LC_NUMERIC_MASK, "C", nullptr); | ||
| if (loc) | ||
| { | ||
| locale_t prev = uselocale(loc); | ||
| uselocale(prev); | ||
| freelocale(loc); | ||
| } | ||
| return 0; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| /*========================================================================= | ||
| * | ||
| * 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 <memory> | ||
|
|
||
| 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 (when newlocale/uselocale are available): Uses thread-local locale | ||
| * - On Windows (when _configthreadlocale is available): Uses thread-specific locale | ||
| * - Fallback (when neither is available): Issues a warning if locale differs from "C", | ||
| * but does not change the locale. Applications must manage locale externally. | ||
| * | ||
| * 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: | ||
| // Forward declaration of implementation structure | ||
| struct Impl; | ||
| // Pointer to implementation (pImpl idiom) | ||
| std::unique_ptr<Impl> m_Impl; | ||
| }; | ||
|
|
||
| } // end namespace itk | ||
|
|
||
| #endif // itkNumericLocale_h |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,150 @@ | ||
| /*========================================================================= | ||
| * | ||
| * 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 "itkConfigurePrivate.h" | ||
|
|
||
| #include <locale.h> | ||
| #include <cstring> | ||
| #include <cstdlib> | ||
|
|
||
| // Include platform-specific headers based on detected features | ||
| #ifdef ITK_HAS_NEWLOCALE | ||
| # if defined(__APPLE__) | ||
| # include <xlocale.h> | ||
| # endif | ||
| #endif | ||
|
|
||
| namespace itk | ||
| { | ||
|
|
||
| // Implementation structure definition | ||
| struct NumericLocale::Impl | ||
| { | ||
| #ifdef ITK_HAS_CONFIGTHREADLOCALE | ||
| // Windows: thread-specific locale | ||
| int m_PreviousThreadLocaleSetting{ -1 }; | ||
| char * m_SavedLocale{ nullptr }; | ||
| #elif defined(ITK_HAS_NEWLOCALE) | ||
| // POSIX: thread-local locale | ||
| locale_t m_PreviousLocale{ nullptr }; | ||
| locale_t m_CLocale{ nullptr }; | ||
| #else | ||
| // Fallback: no locale change, only check and warn | ||
| bool m_WarningIssued{ false }; | ||
| #endif | ||
| }; | ||
|
|
||
| #ifdef ITK_HAS_CONFIGTHREADLOCALE | ||
|
|
||
| // Windows implementation using thread-specific locale | ||
| NumericLocale::NumericLocale() | ||
| : m_Impl(new Impl()) | ||
| { | ||
| // Enable thread-specific locale for this thread | ||
| m_Impl->m_PreviousThreadLocaleSetting = _configthreadlocale(_ENABLE_PER_THREAD_LOCALE); | ||
|
|
||
| // Save current LC_NUMERIC locale | ||
| const char * currentLocale = setlocale(LC_NUMERIC, nullptr); | ||
| if (currentLocale) | ||
| { | ||
| m_Impl->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_Impl->m_SavedLocale) | ||
| { | ||
| setlocale(LC_NUMERIC, m_Impl->m_SavedLocale); | ||
| free(m_Impl->m_SavedLocale); | ||
| } | ||
|
|
||
| // Restore previous thread-specific locale setting | ||
| if (m_Impl->m_PreviousThreadLocaleSetting != -1) | ||
| { | ||
| _configthreadlocale(m_Impl->m_PreviousThreadLocaleSetting); | ||
| } | ||
| } | ||
|
|
||
| #elif defined(ITK_HAS_NEWLOCALE) | ||
|
|
||
| // POSIX implementation using thread-local locale (uselocale/newlocale) | ||
| NumericLocale::NumericLocale() | ||
| : m_Impl(new Impl()) | ||
| { | ||
| // 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_Impl->m_CLocale = newlocale(LC_NUMERIC_MASK, "C", nullptr); | ||
|
|
||
| if (m_Impl->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_Impl->m_PreviousLocale = uselocale(m_Impl->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_Impl->m_CLocale) | ||
| { | ||
| uselocale(m_Impl->m_PreviousLocale); | ||
| freelocale(m_Impl->m_CLocale); | ||
| } | ||
| } | ||
|
|
||
| #else | ||
|
|
||
| // Fallback implementation - only check locale and issue warning if not "C" | ||
| // Do not modify the locale, application must manage it externally | ||
| NumericLocale::NumericLocale() | ||
| : m_Impl(new Impl()) | ||
| { | ||
| // Check if current locale is compatible with expected "C" locale | ||
| const char * currentLocale = setlocale(LC_NUMERIC, nullptr); | ||
| if (currentLocale && std::strcmp(currentLocale, "C") != 0) | ||
| { | ||
| // Issue warning only once per instance | ||
| itkWarningMacro("LC_NUMERIC locale is '" << currentLocale << "' (not 'C'). " | ||
| << "Thread-safe locale functions not available. " | ||
| << "Locale-dependent number parsing may cause issues. " | ||
| << "Please manage locale at application level."); | ||
| m_Impl->m_WarningIssued = true; | ||
| } | ||
| } | ||
|
|
||
| NumericLocale::~NumericLocale() | ||
| { | ||
| // No action needed in fallback - we don't change the locale | ||
| } | ||
|
|
||
| #endif | ||
|
|
||
| } // end namespace itk | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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.