InsightSoftwareConsortium / InsightSoftwareConsortium/ITK
itk::StringTo* helpers accept trailing content after a valid numeric prefix
- 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
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