InsightSoftwareConsortium / InsightSoftwareConsortium/ITK

itk::StringTo* helpers accept trailing content after a valid numeric prefix

Open
#6,698 0 comments 1 reaction 0 assignees View on GitHub
Dominant language
C++
Stars
1.7k
Forks
748
Avg merge
1d 1h
Merged PRs (30d)
64

Description

The `itk::StringTo*` helpers in `Modules/Core/Common/src/itkStringConvert.cxx` accept a valid numeric prefix followed by arbitrary trailing content. `itk::StringToDouble("1.25junk", ctx)` returns `1.25` rather than throwing, because the validity check is "did the conversion start" (`end == begin`) rather than "did the conversion consume the input".

Every IO call site migrated to these helpers in #6089 inherits that leniency, so a malformed header field or DICOM Decimal String can silently yield a plausible-looking value instead of a parse error.

### Current behavior

Measured against `ITKCommon` (macOS arm64), alongside the `std::stod` these helpers replaced:

| Input | `itk::StringToDouble` | `std::stod` |
|---|---|---|
| `"1.25junk"` | `1.25` | `1.25` |
| `"1.25 "` | `1.25` | `1.25` |
| `" 1.25"` | `1.25` | `1.25` |
| `"1.25\2.5"` | `1.25` | `1.25` |
| `"junk"` | throws | throws |
| `"1.25"` | `1.25` | `1.25` |

The behavior is identical to `std::stod`, so it long predates the locale work in #6695; it is inherited, not introduced. `strtod`'s `endptr` contract distinguishes "nothing consumed" from "something consumed" and leaves "partially consumed" to the caller, and `std::stod` chose the lenient reading.

### Why this is not a one-line fix

A naive `*end != '\0'` rejection would break valid input:

- **DICOM DS** explicitly permits leading and trailing spaces, so `"1.25 "` must keep parsing. Any strict check has to skip trailing whitespace before rejecting.
- `itk::StringToDouble` / `StringToFloat` are now the shared funnel for many readers (NRRD, DICOM, and others migrated in #6089). Tightening validation is an input-compatibility change: files that load today could start failing, which is a deliberate decision rather than a drive-by fix.

### Proposal

1. Reject trailing content that is not whitespace, for the whole `itk::StringTo*` family (integer helpers included — `std::stoll`/`std::stoull` are lenient the same way).
2. Keep leading and trailing whitespace valid.
3. Add GTest coverage for the prefix cases: `"1.25junk"`, `"1.25\2.5"`, `"1 2"`, `"0x10"`, and the whitespace cases that must still pass.
4. Survey call sites for readers that legitimately hand these helpers a substring with a delimiter still attached, and fix those to split first rather than relying on the lenient parse.

### Provenance

Raised as a P1 by an automated reviewer on #6695 (`discussion_r3646959273`), attributed there to that PR's `strtod_l` change. Verification against the merge base showed the behavior unchanged from `std::stod`, so it was kept out of #6695's scope and tracked here instead.

Contributor guide

Open the contributing guide

Research direction

Start in Modules/Core/Common/src/itkStringConvert.cxx and inspect the existing conversion tests and all readers migrated in #6089. Run the ITKCommon tests with the listed numeric-prefix and whitespace inputs, then survey call sites for attached delimiters. Done means non-whitespace trailing content is rejected across the StringTo* helpers without breaking valid whitespace or legitimate reader input.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
backend, testing
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
58/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.