Skip to content

stou32() accepts partial parses, unlike the other stringutils converters #4554

Description

@Daksha1611

Describe the bug

ovms::stou32() accepts a string that is only partially a number, unlike every other converter in src/stringutils.cpp.

src/stringutils.cpp:112-129:

std::optional<uint32_t> stou32(const std::string& input) {
    std::string str = input;
    ovms::erase_spaces(str);

    if (!str.empty() && str[0] == '-') {
        return std::nullopt;
    }

    try {
        uint64_t val = std::stoul(str);          // no idx, no full-consumption check
        if (val > std::numeric_limits<uint32_t>::max()) {
            return std::nullopt;
        }
        return {static_cast<uint32_t>(val)};
    } catch (...) {
        return std::nullopt;
    }
}

std::stoul stops at the first character it cannot use and still reports success. Its siblings in the same file all guard against that by passing &idx and comparing against the length:

  • stou64src/stringutils.cpp:143-147
  • stoi32src/stringutils.cpp:161-165
  • stofsrc/stringutils.cpp:203-206

(stoi64 at :181-185 validates by scanning digits first, which is equivalent.) stou32 is the only one without the check, so:

  12abc          -> 12
  3.9            -> 3
  0x10           -> 0
  100%           -> 100

Call sites affected

  1. src/grpcservermodule.cpp:79 — the GRPC_SERVERS environment variable:

    const char* environmentVariableBuffer = std::getenv("GRPC_SERVERS");
    if (environmentVariableBuffer) {
        auto result = stou32(environmentVariableBuffer);
        if (result && result.value() > 0) {
            return result.value();
        }
    }

    GRPC_SERVERS=4x silently starts 4 gRPC servers instead of being rejected and falling back to config.grpcWorkers(). A typo in a deployment environment variable is applied rather than reported.

  2. src/audio/speech_to_text/s2t_servable.cpp:81 — the transcription temperature field, used as a fallback after ovms::stof has already rejected the value. That endpoint's user-visible symptom is filed separately; either fix resolves it independently and the two do not conflict.

To Reproduce

Unit level, alongside the existing StringUtils.stou32 case:

EXPECT_FALSE(ovms::stou32("12abc"));   // currently returns 12
EXPECT_FALSE(ovms::stou32("3.9"));     // currently returns 3
EXPECT_FALSE(ovms::stou32("0x10"));    // currently returns 0

End to end:

GRPC_SERVERS=4x ovms --model_path /models/... --model_name m --port 9000
# starts 4 gRPC servers; expected: the value is rejected and grpcWorkers() is used

Expected behavior

stou32 behaves like stou64/stoi32/stof: a string that is not entirely a number returns std::nullopt.

Logs

None — the value is accepted, so nothing is logged.

Configuration

  1. OVMS version: main @ fadb3314
  2. Any configuration; reachable via the GRPC_SERVERS environment variable
  3. CPU
  4. N/A
  5. N/A

Additional context

Suggested fix, matching the siblings:

size_t idx = 0;
uint64_t val = std::stoul(str, &idx);
if (idx != str.size()) {
    return std::nullopt;
}

The existing StringUtils.stou32 test covers only a negative value, overflow and the maximum, so none of it changes.

Separate, deliberately not bundled: stou32 calls erase_spaces() on its input first, so stou32("12 34") yields 1234 both before and after such a fix, whereas stou64 rejects " 100 " outright (there is an explicit test for that at src/test/stringutils_test.cpp:218). Whether stou32 should also stop erasing interior whitespace is a separate behavioural question and worth deciding on its own.

Also same class, different function, mentioned only so it is on the record: src/grpcservermodule.cpp:143 uses a bare std::stoi(value) to decide whether a gRPC channel argument is an integer, so grpc.max_receive_message_length=4MB is passed to gRPC as the integer 4 rather than as the string it is. That may well be intended, given the surrounding comment, so I have not touched it.

I have a patch for this and will open a PR shortly.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions