Repository navigation
Conversation
github-actions
Bot
added
tests
util-lib
please-review
|
Reviewed 2f6a645 (architecture, correctness, security, style, tests; failed: lifetimes). Suggestions
The review is incomplete, so the next push reviews the full diff again. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| /* safe_atolu_full() accepts "-0", hence refuse the "-" explicitly, so that e.g. "-0.50" is not | ||
| * taken as 0.50. */ | ||
| if (strchr(s, '-')) | ||
| return -ERANGE; |
There was a problem hiding this comment.
Claude: suggestion: Rather than scanning s for '-' after parsing, pass 10 | SAFE_ATO_REFUSE_PLUS_MINUS (plus SAFE_ATO_REFUSE_LEADING_WHITESPACE for f_str) to both safe_atolu_full() calls. That flag is the usual way to refuse a sign, and it also refuses "1.+5" and "1. 5". It returns EINVAL, so the "-4000.5" test needs adjusting.
There was a problem hiding this comment.
Good point, done in ed26871: both calls now pass 10 | SAFE_ATO_REFUSE_PLUS_MINUS (the fraction also SAFE_ATO_REFUSE_LEADING_WHITESPACE) and the strchr() scan is gone. This also refuses "+1.5", "1.+5" and "1. 5", which the scan let through. The negative cases now return -EINVAL, so the test expectations are adjusted and the new refusals are covered by tests.
| f_str = d + 1; | ||
|
|
||
| r = safe_atolu_full(i_str, 10, &i); | ||
| r = safe_atolu_full(i_str, 10 | SAFE_ATO_REFUSE_PLUS_MINUS, &i); |
There was a problem hiding this comment.
|
Please rebase to fix the Lint Code Base failure. Also, see the comment by Claude. |
yuwata
added
reviewed/needs-rework 🔨
The percent/permille/permyriad parsers parse the integer part with
safe_atoi() and then refuse negative results. However, safe_atoi()
parses "-0" as 0, hence a negative value whose integer part is zero
slipped through, and the sign got lost when the fractional part was
added. E.g. parse_permyriad("-0.5%") returned 50 (i.e. 0.5%), while
parse_permyriad("-7%") correctly failed with ERANGE. As a result,
settings such as CPUQuota=-0.5% or ManagedOOMMemoryPressureLimit=-0.5%
were silently accepted as positive values.
Let's refuse a "-" in the integer part explicitly, so that all
negative values are rejected with ERANGE.
parse_loadavg_fixed_point() parses both parts of the value with safe_atolu_full(), which refuses negative numbers with ERANGE, except for "-0". Hence "-4000.5" was refused, but "-0.50" was accepted as 0.50 and "1.-0" as 1.00. Pass SAFE_ATO_REFUSE_PLUS_MINUS to both calls, plus SAFE_ATO_REFUSE_LEADING_WHITESPACE for the fractional part, so that any sign is refused up front. This also refuses "+1.5", "1.+5" and "1. 5". Negative values now fail with EINVAL, as documented for the flag, and the test expectations are adjusted accordingly.
ed26871 to
c5e7f0f
Compare
|
Rebased onto current main and squashed the follow-up into the parse-util commit; the commit message now describes the SAFE_ATO_REFUSE_PLUS_MINUS approach. |
github-actions
Bot
added
please-review
|
one of those days we should just stop use ato*l() altogether. we are adjusting so much around it, that the question is why not just do the thing entirely on our own... (this is not a request to implement any patch to get rid of ato*l() use, it's just a general comment) |
| return -EINVAL; | ||
|
|
||
| n = strndupa_safe(p, pc - p); | ||
| r = safe_atoi(n, &v); |
There was a problem hiding this comment.
why not just use safe_atoi_full(n, 10 | SAFE_ATO_REFUSE_PLUS_MINUS | SAFE_ATO_REFUSE_LEADING_ZERO, v) here?
i mean, that'd be more correct anyway, since we do not want to allow hex or oct here...
There was a problem hiding this comment.
Done in 2f6a645. One adjustment: there is no safe_atoi_full() in the tree (only safe_atoi(), which takes no flags), so I used safe_atou_full() with the base and flags you suggested (10 | SAFE_ATO_REFUSE_PLUS_MINUS | SAFE_ATO_REFUSE_LEADING_ZERO), plus an INT_MAX check to keep the previous range behavior. Hex/octal spellings and leading zeros are rejected now too. One visible consequence: signed inputs now fail with -EINVAL instead of -ERANGE, and I updated the test expectations accordingly. test-percent-util and test-parse-util pass.
poettering
added
reviewed/needs-rework 🔨
…part Refuse signs, leading zeros and non-decimal spellings (hex/octal) by parsing the integer part with safe_atou_full() in base 10 with SAFE_ATO_REFUSE_PLUS_MINUS|SAFE_ATO_REFUSE_LEADING_ZERO, as suggested in review. Signed inputs now fail with -EINVAL instead of -ERANGE; update the test expectations accordingly.
github-actions
Bot
added
please-review
The percent, permille and permyriad parsers parse the integer part with safe_atoi() and refuse negative results, but safe_atoi() parses "-0" as 0, so a negative value with a zero integer part slipped through and its sign was lost once the fractional part was added: parse_permyriad("-0.5%") returned 50, while "-7%" is refused with ERANGE. As a result, CPUQuota=-0.5% was silently applied as a 0.5% quota. The first commit refuses a "-" in the integer part explicitly; this also refuses "-0%" itself, consistent with parse_time(). The second commit fixes the same hole in parse_loadavg_fixed_point(), where "-0.50" was accepted as 0.50.
I ran the tests locally on Linux: the new cases in test-percent-util and test-parse-util fail without this change and pass with it, and test-conf-parser and test-psi-util still pass.
Disclosure: this patch was prepared with AI assistance. I reviewed every line myself and ran the tests above.