parse-util: fix ineffective overflow guards in store_loadavg_fixed_point() (#43753)
`store_loadavg_fixed_point()` guarded both of its `<<
LOADAVG_PRECISION_BITS` shifts against overflow, but neither guard was
effective:
- The integer part was rejected with `i >= (~0UL <<
LOADAVG_PRECISION_BITS)`. The shift direction is inverted: that bound is
`0xFFFFFFFFFFFFF800`, so it only rejects the topmost 2048 values of the
range, while the smallest value that actually overflows `i << 11` is
`2^53`. Everything from `2^53` up shifted out of the word and was
accepted.
- The fractional part was shifted first and range-checked afterwards, so
a large `f` wrapped to a small value and then passed the `f >=
LOADAVG_FIXED_POINT_1_0` test.
Concretely, on 64 bit:
```
parse_loadavg_fixed_point("9007199254740992.00", &fp) -> 0, fp == 0 (load reported as 0.00)
parse_loadavg_fixed_point("1.9007199254740992", &fp) -> 0, fp == 1.00
```
Both should be `-ERANGE`. The existing
`ASSERT_ERROR(..."18446744073709551615.5"..., ERANGE)` passes only
because `~0UL` happens to fall inside the narrow band the broken guard
does catch.
Validate both parts before shifting instead. Rejecting `f >= 100` is
exactly equivalent to the old post-shift comparison for every input that
previously succeeded, since `DIV_ROUND_UP(99 << 11, 100) == 2028` and
`DIV_ROUND_UP(100 << 11, 100) == 2048`, so there is no behaviour change
for valid input; only the values that used to wrap are now rejected.
Note this is a latent bug rather than a live one: the only caller of
`parse_loadavg_fixed_point()` is `read_resource_pressure()`, which
parses kernel-generated `/proc/pressure/*` and cgroup `memory.pressure`
files where values are capped at `100.00`, and every direct
`store_loadavg_fixed_point()` caller passes permyriad-derived values
already clamped to 0...10000. Fixing the guards keeps the helper correct
for future callers.
Covered by new cases in `test-parse-util`, including the largest integer
part that must still be accepted (`2^53-1`).
This commit is contained in:
@@ -779,16 +779,17 @@ int parse_oom_score_adjust(const char *s, int *ret) {
|
||||
int store_loadavg_fixed_point(unsigned long i, unsigned long f, loadavg_t *ret) {
|
||||
assert(ret);
|
||||
|
||||
if (i >= (~0UL << LOADAVG_PRECISION_BITS))
|
||||
/* Validate both parts *before* shifting them, so that the shifts themselves cannot overflow.
|
||||
* Rejecting f >= 100 is equivalent to the old check of the shifted value against
|
||||
* LOADAVG_FIXED_POINT_1_0, since DIV_ROUND_UP(99 << 11, 100) == 2028 and
|
||||
* DIV_ROUND_UP(100 << 11, 100) == 2048. */
|
||||
if (i > (~0UL >> LOADAVG_PRECISION_BITS))
|
||||
return -ERANGE;
|
||||
|
||||
i = i << LOADAVG_PRECISION_BITS;
|
||||
f = DIV_ROUND_UP((f << LOADAVG_PRECISION_BITS), 100);
|
||||
|
||||
if (f >= LOADAVG_FIXED_POINT_1_0)
|
||||
if (f >= 100)
|
||||
return -ERANGE;
|
||||
|
||||
*ret = i | f;
|
||||
*ret = (i << LOADAVG_PRECISION_BITS) | DIV_ROUND_UP(f << LOADAVG_PRECISION_BITS, 100);
|
||||
return 0;
|
||||
}
|
||||
|
||||
|
||||
@@ -7,6 +7,8 @@
|
||||
#include "capability-util.h"
|
||||
#include "locale-util.h"
|
||||
#include "parse-util.h"
|
||||
#include "stdio-util.h"
|
||||
#include "string-util.h"
|
||||
#include "tests.h"
|
||||
|
||||
TEST(parse_boolean) {
|
||||
@@ -864,6 +866,26 @@ TEST(parse_loadavg_fixed_point) {
|
||||
ASSERT_ERROR(parse_loadavg_fixed_point("4096.4096", &fp), ERANGE);
|
||||
ASSERT_ERROR(parse_loadavg_fixed_point("-4000.5", &fp), ERANGE);
|
||||
ASSERT_ERROR(parse_loadavg_fixed_point("18446744073709551615.5", &fp), ERANGE);
|
||||
|
||||
/* Neither part may be large enough to overflow the shift by LOADAVG_PRECISION_BITS. Derive the
|
||||
* boundary from the width of unsigned long, so that this holds on 32 bit too: the largest integer
|
||||
* part that still fits is ~0UL >> LOADAVG_PRECISION_BITS (2^53-1 on 64 bit), and one more than
|
||||
* that used to shift clean out of the word and be reported as a load of zero. */
|
||||
unsigned long max_side = ~0UL >> LOADAVG_PRECISION_BITS;
|
||||
char buf[DECIMAL_STR_MAX(unsigned long) + STRLEN(".99")];
|
||||
|
||||
xsprintf(buf, "%lu.99", max_side);
|
||||
ASSERT_OK_ZERO(parse_loadavg_fixed_point(buf, &fp));
|
||||
ASSERT_EQ(LOADAVG_INT_SIDE(fp), max_side);
|
||||
ASSERT_EQ(LOADAVG_DECIMAL_SIDE(fp), 99U);
|
||||
|
||||
xsprintf(buf, "%lu.00", max_side + 1);
|
||||
ASSERT_ERROR(parse_loadavg_fixed_point(buf, &fp), ERANGE);
|
||||
|
||||
/* Same for the fractional part, which used to be shifted before it was range checked. */
|
||||
xsprintf(buf, "1.%lu", max_side + 1);
|
||||
ASSERT_ERROR(parse_loadavg_fixed_point(buf, &fp), ERANGE);
|
||||
|
||||
ASSERT_ERROR(parse_loadavg_fixed_point("foobar", &fp), EINVAL);
|
||||
ASSERT_ERROR(parse_loadavg_fixed_point("3333", &fp), EINVAL);
|
||||
ASSERT_ERROR(parse_loadavg_fixed_point("1.2.3", &fp), EINVAL);
|
||||
|
||||
Reference in New Issue
Block a user