Repository navigation
Reject partial parses in stou32 - #4555
Merged
Merged
Conversation
stou32() is the only converter in stringutils that does not verify the whole
string was consumed. std::stoul stops at the first character it cannot use
and still reports success, so:
stou32("12abc") == 12
stou32("3.9") == 3
stou32("0x10") == 0
stou32("100%") == 100
Its siblings all guard against this by passing &idx and comparing against
the length - stou64 at :143-147, stoi32 at :161-165, stof at :203-206.
(stoi64 at :181-185 validates by scanning digits first, which is
equivalent.) Add the same check.
Call sites this affects:
grpcservermodule.cpp:79 - the GRPC_SERVERS environment variable.
GRPC_SERVERS=4x silently started 4 servers instead of being rejected
and falling back to config.grpcWorkers().
s2t_servable.cpp:81 - the transcription temperature field, where the
value is used as a fallback after ovms::stof has already rejected it.
The surrounding erase_spaces() call is deliberately left alone, so
stou32("12 34") still yields 1234. That is a separate question from partial
parsing - note stou64 rejects " 100 " outright - and changing it would be
a second behaviour change in one commit.
Tests: the existing StringUtils.stou32 case only covers a negative value,
overflow and the maximum, none of which change. Adds
StringUtils.stou32RejectsPartialParse for the trailing-garbage spellings
above, plus the plain-number cases to pin that valid input still parses.
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The implementation and regression coverage address the issue; only a minor wording nit remains.
Review effort: Lite
Findings: None
What changed in this PR
Fixes stou32() to reject partially parsed numeric strings, matching sibling converters.
Changes:
- Added full-consumption validation to
stou32. - Added regression tests for malformed and valid inputs.
| File | Description |
|---|---|
src/test/stringutils_test.cpp |
Adds coverage for partial-parse rejection and valid values. |
src/stringutils.cpp |
Rejects inputs not fully consumed by std::stoul. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
mzegla
approved these changes
Oct 7, 2026
3 tasks
mzegla
pushed a commit
that referenced
this pull request
Oct 19, 2026
…it (#4553) ### 🛠 Summary Fixes #4552. `parseTemperature()` parsed the multipart `temperature` field with `ovms::stof` and, on failure, retried with `ovms::stou32`. `ovms::stof` requires the whole field to be consumed; `ovms::stou32` does not, and it erases spaces first. The fallback therefore rescued exactly the values `stof` had just rejected, turning malformed input into a plausible temperature instead of the intended `Invalid temperature type.` error: ``` temperature=0.5x -> accepted as 0.0, not 0.5, so sampling is silently off temperature=0abc -> accepted as 0.0 temperature=1e400 -> accepted as 1.0 temperature=5 5 -> accepted as 55.0 ``` The fallback had no purpose — `ovms::stof` already parses integer spellings, so `"1"` never reached it. Every input it could rescue is malformed by definition. Removing it lets the existing error path fire. This deliberately adds no range check. Passing a negative temperature through for GenAI to reject is the behaviour established by #4201 and pinned by `negativeTemperatureEnableSampling`, so I left it alone rather than quietly reverse it. Happy to revisit separately if the endpoint should enforce the OpenAI audio range at the parse layer. Adds four cases for the spellings above. The three existing `SttServableParseTemperatureTest` cases are unaffected — `-1.0`, `0` and `1.0` all parse cleanly through `ovms::stof` and never reached the fallback. The root cause is that `ovms::stou32` alone among the `stringutils` converters does not check for a partial parse; that is #4554, and the two fixes are independent. Since #4555 merged, stou32 rejects partial parses itself, so on main only "5 5" is still accepted (stou32 strips the space and parses 55). The other three values stay as regression tests. I have no OVMS build container available, so this is not compiled against the full tree and CI will need to confirm the build. I did check the change in isolation and verified the three existing test inputs behave identically. Draft for that reason. ### 🧪 Checklist - [ ] Unit tests added. - [ ] The documentation updated. - [ ] Change follows security best practices. --------- Co-authored-by: michalkulakowski <Michal.Kulakowski@intel.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
🛠 Summary
Fixes #4554.
stou32()is the only converter instringutilsthat does not verify the whole string was consumed.std::stoulstops at the first character it cannot use and still reports success, sostou32("12abc")returned 12,stou32("3.9")returned 3 andstou32("0x10")returned 0. Its siblings all guard against this by passing&idxand comparing against the length —stou64at:143-147,stoi32at:161-165,stofat:203-206. This adds the same check.Two call sites are affected.
grpcservermodule.cpp:79reads theGRPC_SERVERSenvironment variable, soGRPC_SERVERS=4xsilently started 4 servers instead of being rejected and falling back toconfig.grpcWorkers().s2t_servable.cpp:81uses it as a fallback for the transcription temperature field; that endpoint is also addressed in #4553, and either change alone fixes it — they do not conflict.The surrounding
erase_spaces()call is deliberately left alone, sostou32("12 34")still yields 1234. That is a separate question from partial parsing — notestou64rejects" 100 "outright — and I did not want two behaviour changes in one commit.The existing
StringUtils.stou32case covers a negative value, overflow and the maximum, none of which change. AddsStringUtils.stou32RejectsPartialParsefor the trailing-garbage spellings plus plain numbers.I have no OVMS build container available, so this is not compiled against the full tree and CI will need to confirm the build. I did check the change in isolation and confirmed the existing test inputs are unaffected. Draft for that reason.
🧪 Checklist