[CODE HEALTH] Fix clang-tidy bugprone-unchecked-string-to-number-conversion warnings#4216
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4216 +/- ##
==========================================
- Coverage 82.83% 82.83% -0.00%
==========================================
Files 415 415
Lines 17423 17431 +8
==========================================
+ Hits 14431 14437 +6
- Misses 2992 2994 +2
🚀 New features to boost your workflow:
|
dbarker
left a comment
dbarker
left a comment
There was a problem hiding this comment.
Thanks for the PR! Please see comment below for a minor change.
thc1006
force-pushed
the
codehealth/unchecked-string-to-number
branch
from
1f6ec12 to
19ee370
Compare
thc1006
force-pushed
the
codehealth/unchecked-string-to-number
branch
from
19ee370 to
b267168
Compare
|
Done. Applied it to all the strtol calls in the PR (not just this line) so they stay consistent, and switched those files from |
dbarker
left a comment
dbarker
left a comment
There was a problem hiding this comment.
Thanks for the cleanup. Approved, with some optional suggestions for this PR since the old behavior is retained.
| if (colon) | ||
| { | ||
| inet4.sin_port = htons(atoi(colon + 1)); | ||
| inet4.sin_port = htons(static_cast<uint16_t>(std::strtol(colon + 1, nullptr, 10))); |
There was a problem hiding this comment.
consider adding a check for a valid port (errno, end ptr, returned value in range) before casting to uint16_t
There was a problem hiding this comment.
Added, thanks. It now checks errno, the end pointer, and the 0-65535 range, and falls back to 0 if any of those fail.
| if (contentLength != conn.request.headers.end()) | ||
| { | ||
| conn.contentLength = atoi(contentLength->second.c_str()); | ||
| conn.contentLength = |
There was a problem hiding this comment.
consider adding a check for a valid return from std::strtol before casting and setting content length. Can be a follow up PR.
There was a problem hiding this comment.
Done here rather than a follow-up. It now checks errno and the end pointer and rejects a negative value with 400 Bad Request. I left trailing characters after a valid number tolerated, since the header value parser does not strip trailing whitespace and I did not want to reject something like 100 .
…ersion warnings Replace unchecked atoi/sscanf calls with std::strtol across 8 sites (explicit casts preserve the original result type, so no narrowing warning is introduced), and NOLINT the one sscanf whose return count is already validated. Lower the clang-tidy warning_limit accordingly. The two ext/http server sites that parse untrusted input (the address port in socket_tools.h and Content-Length in http_server.h) now validate the std::strtol result (errno, end pointer, and range) before use; an invalid Content-Length is answered with 400 Bad Request. Part of open-telemetry#2053. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
thc1006
force-pushed
the
codehealth/unchecked-string-to-number
branch
from
b267168 to
724f289
Compare
Changes
Resolves the
bugprone-unchecked-string-to-number-conversionwarnings (part of #2053) by replacing uncheckedatoi/sscanfcalls withstrtol, as the check recommends.atoicall sites switched tostrtol(with an explicit cast that preserves the original result type, so no narrowing-conversion warning is introduced).<cstdlib>is added to the two ext/http headers that now usestrtoldirectly.sscanfsite inconfiguration_parser.ccis annotated withNOLINTNEXTLINE, since its return count is already validated on the next line (astrtolrewrite would be redundant there).No behavior change:
atoi(s)andstrtol(s, nullptr, 10)parse identically for valid input.Verification
warning_limitin.github/workflows/clang-tidy.yamlis lowered from 309/319 to 300/310 (the current CI baseline minus the 9 resolved).