BUG: Fix locale-dependent parsing in NRRD reader causing metadata corruption

[Copilot]

Added Google Test for NumericLocale per @blowekamp and @dzenanz (commit 0fc849d):

Test coverage:

• Verifies temporary setting to C locale
• Tests float parsing with dot decimal separator
• Tests nesting behavior
• Tests with German locale (de_DE.UTF-8) if available
• Tests sequential uses
• Tests basic RAII cleanup

All 6 tests pass.

Original prompt

---

This section details on the original issue you should resolve

<issue_title>Locale-Dependent Parsing in ITK NRRD Reader Causes Silent Metadata Corruption</issue_title>
<issue_description>### Description
ITK’s NRRD reader parses floating-point metadata (such as spacing, direction vectors, and other numeric header fields) using locale-dependent number parsing (strtod).
In numeric locales where the decimal separator is a comma (for example de_DE.UTF-8, common in many European countries), values containing a dot such as 0.878906 are parsed incorrectly. In such locales, strtod("0.878906") yields 0.0.

This problem leads to two kinds of failures:

1. Silent metadata corruption (no error raised)
   Values with fractional parts greater than 1 (for example 3.5, 2.2) may be misparsed (fractional part ignored) without causing an error. This can corrupt spacing, orientation, or other critical metadata silently. The image loads and all downstream computations use incorrect metadata.

2. Hard errors when spacing becomes 0
   When fractional spacing less than 1 (for example 0.878906 or 0.8) is parsed as 0.0, ITK sometimes throws
   Zero-valued spacing is not supported.
   This error exposes the bug, but only for particular values. For many other metadata fields and values the corruption is completely silent.

The same issue was already reported here, but was never resolved:
#3375

A similar issue was previously identified and fixed for VTK files:
#2297

Impact

This issue can silently corrupt metadata when reading NRRD files on systems with non-English numeric locales. This includes:

• space directions
• space origin
• spacing
• measurements encoded in metadata
• values in DICOM-derived metadata fields stored in NRRD
• any numeric field parsed through locale-dependent routines

This is particularly problematic in medical imaging, where spacing, orientation, and geometric metadata directly affect:

• registration
• segmentation
• dose calculation
• physical measurement interpretation
• reconstruction algorithms

The most serious aspect is that metadata can be corrupted without any warning or error message. The bug was only discovered because in some cases spacing becomes exactly zero, triggering ITK’s Zero-valued spacing is not supported check. In many other cases (for example when only the fractional part is lost, or when values are truncated but remain positive) the corruption is completely silent and can remain undetected.

The issue is typically triggered only when the host application explicitly applies the system locale, which is common in GUI frameworks such as Qt. This is why the bug appears in some environments (for example napari or other Qt-based tools) while plain C++ programs often appear unaffected.

Root Cause

Many GUI frameworks, such as Qt, call:

setlocale(LC_ALL, "");

to apply the system locale. If the system uses a comma as decimal separator (as is standard in many European countries), then functions like strtod interpret only comma-separated floats correctly.

Example:

• In C locale: strtod("0.878906") → 0.878906
• In de_DE.UTF-8 locale: strtod("0.878906") → 0.0

Thus, a valid NRRD header field such as:

space directions: (0.878906,0,0) (0,3,0) (0,0,3)

may be parsed by ITK as something like:

(0.0, 0, 0)
(0, 3, 0)
(0, 0, 3)

If the corrupted value results in spacing zero, ITK throws an error.
If the corrupted value remains positive (for example if only the fractional part is dropped), the metadata is accepted but wrong, and no error is raised.

Minimal Reproducible Examples (Python, self-contained)

The following examples assume that the de_DE.UTF-8 locale is installed on the system.
They demonstrate both the silent corruption case and the zero-spacing error case, using only Python and ITK. Each example:

1. Creates a random 3D numpy array.
2. Writes it to NRRD using ITK under a safe numeric locale (C).
3. Switches the numeric locale to de_DE.UTF-8.
4. Reads the same NRRD with ITK and inspects the spacing.

Depending on the actual parsing behavior, the spacing may be silently wrong or may cause an exception.

Note: depending on how the ITK Python wrappers format spacing, component order in printing may appear as (z, y, x) or (x, y, z), but the key observation is the incorrect handling of fractional parts under de_DE numeric locale.

1. Silent metadata corruption example (spacing > 1)

import os
import locale
import pathlib
import numpy as np
import itk

# Ensure we start in a safe numeric locale for writing
locale.setlocale(locale.LC_NUMERIC, "C")

# Create a small test image and write it to NRRD with fractional spacing...

</details>



<!-- START COPILOT CODING AGENT SUFFIX -->

- Fixes InsightSoftwareConsortium/ITK#5683

<!-- START COPILOT CODING AGENT TIPS -->
---

💬 We'd love your input! Share your thoughts on Copilot coding agent in our [2 minute survey](https://gh.io/copilot-coding-agent-survey).

[hjmjohnson]

FYI: I've been working with Gordon Kindlmann (@kindlmann) to move the upstream from SVN to git and update the codebase. Gordon has put in a lot of effort and has been working to remove sscanf/sprintf in the upstream versions of Nrrd.

#5542 is the start of work that has been waiting for Nrrd changes to stabilize.

To get Teem: git clone http://git.code.sf.net/p/teem/teem.git teem-src
To get NrrdIO: git clone http://git.code.sf.net/p/teem/nrrdio/nrrdio.git nrrdio-src

[dzenanz]

@copilot address my review feedback.

[dzenanz]

This looks much better. Let's see whether this makes the test pass.

[thewtex]

@copilot rebase and re-organize the commits so:

• There is one commit that adds the tests
• There is another commit that makes the changes to itkNrrdImageIO.cxx

So the changes to itkNrrdImageIO.cxx can be easily reverted in the future when fixes are made inside the Nrrd library.

Use the standard "BUG: " prefix for ITK commit subject lines.

[Copilot]

@copilot rebase and re-organize the commits so:

• There is one commit that adds the tests
  ...

Reorganized commits as requested in commits 7726c6d and 4b120ca:

Commit 1 (7726c6d): BUG: Add test for locale-dependent parsing in NRRD reader

• Only test files (CMakeLists.txt + itkNrrdLocaleTest.cxx)

Commit 2 (4b120ca): BUG: Fix locale-dependent parsing in NRRD reader

• Only the fix in itkNrrdImageIO.cxx

Both commits use "BUG: " prefix. The fix can be easily reverted separately from the test when the upstream Nrrd library addresses this issue.

Note: The commits diverged from remote due to reorganization. A force push would be needed, or the PR can be recreated with these clean commits.

[blowekamp]

Modifying the global locale state is not concurrent thread safe.

There is some discussion here: https://stackoverflow.com/questions/4057319/is-setlocale-thread-safe-function about alternatives that provide thread local settings. It may be OS specific, and not portable. Likely a new ITK utility class is needed. I believe setting the locale is unsafely don't with GDCM too.

[thewtex]

Modifying the global locale state is not concurrent thread safe.

There is some discussion here: https://stackoverflow.com/questions/4057319/is-setlocale-thread-safe-function about alternatives that provide thread local settings. It may be OS specific, and not portable. Likely a new ITK utility class is needed. I believe setting the locale is unsafely don't with GDCM too.

@copilot attempt to implement an itk class that sets and restores the locale in a thread-safe way.

[Copilot]

Modifying the global locale state is not concurrent thread safe.

There is some discussion here: https://stackoverflow.com/questions/4057319/is-setlocale-thread-safe-function about alternative...

Implemented thread-safe locale handling in commit c9b1302.

Created itk::NumericLocale utility class in ITKCommon with platform-specific implementations:

• POSIX systems (Linux, macOS, BSD): Uses newlocale()/uselocale() for thread-local locale
• Windows: Uses _configthreadlocale() for thread-specific locale
• Fallback: Mutex-protected global setlocale() for other platforms

The NRRD reader now uses this thread-safe utility instead of the local implementation. The class is designed to be reusable by other I/O modules (GDCM, VTK, etc.) that need locale-independent number parsing.

[kindlmann]

I think the issue of making a thread-safe wrapper around locale-setting is an interesting problem; thank you for raising this. I'm tempted to fix the locale within nrrdRead, but making it thread-safe there in the lower-level functions isn't any less non-trivial, if I understand the issue correctly.

Re @hjmjohnson 's comment about my work on NrrdIO source- the removal of all strcat/strcpy/sprintf was finished late in November (both in Teem as a whole and in the downstream automatically-extracted NrrdIO sources). Ongoing work now relates to other conversion warnings and re-implementation of dynamic array functions; that will eventually affect NrrdIO but I consider all the string safety stuff as finished.

[blowekamp]

@kindlmann I did an AI fueled hack to prototype what modifying NRRD to use stream with locales instead of sscanf here might be: #5697

The current state of this PR, does thread local setting of the locale for the common OSs. However it falls back to using a mutex to block other usages of the NumericLocale class. This could make IO nearly single threader for this case, and it's not fully effective with other application logic changing the locale.

There may be application solution to set the locale what would not have the potential consequences of this change. Perhaps we can wait until it's addressed in the NRRD library?

[thewtex]

@copilot apply changes based on the Copilot review comments in this thread

[Copilot]

@copilot apply changes based on the Copilot review comments in this thread

Applied all code review suggestions in commit 461a8a7:

• Fixed POSIX destructor to always restore locale when m_CLocale is set (handles LC_GLOBAL_LOCALE correctly)
• Added documentation for strdup/newlocale failure handling
• Added noexcept to fallback destructor and documented setlocale exception safety
• Fixed issue number placeholder to 5683
• Clarified test comments to explain behavior "without the fix"

All changes tested successfully with both C and de_DE.UTF-8 locales.

[kindlmann]

@blowekamp thanks for the context about Nrrd being used in multi-threaded learning contexts.

As for @hjmjohnson mention of "remove sscanf/sprintf in the upstream versions of Nrrd"; I think that may be little imprecise. What I did finish was removing all use of scary C string functions strcat/strcpy/sprintf (I replaced then with things like snprintf). But sscanf, as a utility of parsing floating point values, was not something I strived to fix.

The issue of locale-dependence is something I was vaguely aware of but this thread has brought the issue into focus. It is not something I had previously prioritized for ongoing Teem/NrrdIO hacking, so I don't now have a specific plan for fixing it (in contrast to @blowekamp optimistically saying "Perhaps we can wait until it's addressed in the NRRD library?"). Still, I do now have a hunch that needs exploring ...

In the mean time, is the consensus here that an Thread-safe RAII class for temporary LC_NUMERIC locale setting is not possible, or just not performant?

[kindlmann]

If I were to pursue a way of fixing locale-sensitivity in NrrdIO, it would help me to answer two questions:

1. Is sscanf_l available on all the machines that ITK wants to compile on? That version of sscanf takes an explicit locale as an extra argument.

2. Beyond de_DE.UTF-8, do you have other examples of locales that break NRRD reading? I would want such examples as part of testing my hunch about side-stepping sscanf for parsing floating-point numbers.

[dzenanz]

Visual Studio seems to have _sscanf_l.

[kindlmann]

By the way, all the same issues with sscanf for reading NRRD header also apply to using of printf to write NRRD headers, but I haven't seen that raised in this thread.

Okay @dzenanz , looking more at this:

Visual Studio seems to have _sscanf_l.

it's more annoying than I first realized: it looks like I'd have to (within NrrdIO) contrive some wrapper functions/macros to bridge the difference between the window-specific _sscanf_l (which takes a _locale_t) vs the rest of the world's sscanf_l (which takes a locale_t).

Can anyone point out if there's precedent in ITK code (third party or otherwise) for this kind of locale wrapper, possibly combined with CMake check_symbol_exists(printf_l) and check_symbol_exists(_printf_l)?

[blowekamp]

Can anyone point out if there's precedent in ITK code (third party or otherwise) for this kind of locale wrapper, possibly combined with CMake check_symbol_exists(printf_l) and check_symbol_exists(_printf_l)?

ITK is not using any of these C _l method that I have found, nor have I found them widely used in other projects. I have see the C++ stream locale used a few places. We should break this updates to NRRD discussion into a separate location.

EDIT: @kindlmann ITK does use the double convert library.

I don't think there is going to be a native NRRD solution soon. And this locale-dependent RAII can be use fully in other IO classes.

IMHO, I thin if these thread safety method are not available then we should issue a compile and runtime warning, that alternate locale are not supported in the ITK runtime and it should be manage at the application level.

I am going to write some instruction for co-polit to follow to make this change, hopefully @thewtex can direct which comment's of mine to follow.

[blowekamp]

Suggested changes to copilot: use try compile, a private implementation, and make the fallback behavior only produce a warning.

[blowekamp]

Please remove all system dependent include in the header. Add a forward declaration for a structure for a pointer to implementation or private implementation.

[blowekamp]

Remove the system dependent code. Use a C++ smart point to an implementation structure.

[blowekamp]

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.

[blowekamp]

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..

[dzenanz]

@copilot apply @blowekamp's suggestions

[Copilot]

@copilot apply @blowekamp's suggestions

Applied all @blowekamp suggestions in commit 24262e2:

• pImpl pattern: Removed all platform-specific includes from header (windows.h, xlocale.h), using forward-declared Impl struct with std::unique_ptr
• CMake feature detection: Added try_compile tests for newlocale/uselocale and _configthreadlocale, with ITK_HAS_NEWLOCALE and ITK_HAS_CONFIGTHREADLOCALE defines in itkConfigurePrivate.h
• Warn-only fallback: When no thread-safe methods available, only issues itkWarningMacro if locale != "C", doesn't modify it or use mutex
• CMake warning: Warns at configuration time if neither method detected

Successfully tested on Linux (detected ITK_HAS_NEWLOCALE) with both C and de_DE.UTF-8 locales.

[dzenanz]

The way copilot implemented this, makes PIMP pointless. Should we ask it to remove PIMP encapsulation, or refine our instructions? Otherwise, this seems to work.

[blowekamp]

The way copilot implemented this, makes PIMP pointless. Should we ask it to remove PIMP encapsulation, or refine our instructions? Otherwise, this seems to work.

It does not look pointless to me. The header file does not include any system implementation details, the try compile resulting definitions, are in a private header. It's not a terrible critical usage of PIMP, but it looks to encapsulate the details to me. I am I missing something?

[dzenanz]

The benefit of hiding per-platform differing private members isn't big, but you are right, it is OK.

[dzenanz]

Time to squash and fix whitespace? And do another round of reviews?

[blowekamp]

I'd like to see a Google Test for the new class in the Common module added.

[dzenanz]

@copilot add a Google Test for the new class in the Common module.

[dzenanz]

I'd like to see a Google Test for the new class in the Common module added.

I believe you could have directly instructed copilot to do this 😄

[Copilot]

@copilot add a Google Test for the new class in the Common module.

Added Google Test for NumericLocale class in commit 0fc849d.

The test suite includes 6 tests:

• TemporarilySetsToCLocale - Verifies locale is set to "C" and restored
• ParsesFloatsWithDotDecimalSeparator - Tests parsing of "3.14159", "0.878906", "2.5"
• SupportsNesting - Tests nested NumericLocale instances
• WorksWithDifferentInitialLocale - Tests with de_DE.UTF-8 locale (skips if unavailable)
• SupportsSequentialUses - Tests multiple sequential uses
• BasicRAII - Tests RAII cleanup behavior

All tests pass successfully (6/6 passed when de_DE.UTF-8 is available, 5/6 passed with 1 skipped when unavailable).

[blowekamp]

I'd like to see a Google Test for the new class in the Common module added.

I believe you could have directly instructed copilot to do this 😄

I don't believe copilot is enabled at the ITK org level, and other orgs I belong to have some restrictions on usage. When I tired it did not listen to me.