Meter: Prevent integer overflow on x positions #1649
Conversation
Explorer09
force-pushed
the
meter-integer-overflow
branch
from
b268b5b to
886dbd1
Compare
Meter.c
Outdated
| // Prevent (x + w) overflow | ||
| assert(x >= 0); | ||
| x = MAXIMUM(0, x); | ||
| assert(w <= INT_MAX - x); | ||
| w = MINIMUM(INT_MAX - x, w); |
There was a problem hiding this comment.
I was considering whether this clamping of x and w is really necessary. While the two assert lines are useful, it might be easier to guard against overflow on the callers of the Meter.draw() functions, rather than guard on the callees.
There are only four functions that would compute x, y, and w values for each meter and call Meter.draw():
Header_drawCPUMeterCommonDrawSingleColCPUsMeter_drawMemorySwapMeter_draw
Explorer09
force-pushed
the
meter-integer-overflow
branch
4 times, most recently
from
92ce490 to
da6d3c2
Compare
Explorer09
force-pushed
the
meter-integer-overflow
branch
from
da6d3c2 to
209e6de
Compare
Explorer09
force-pushed
the
meter-integer-overflow
branch
from
209e6de to
7ff6631
Compare
BenBE
left a comment
BenBE
left a comment
There was a problem hiding this comment.
Some simplifications possible.
Also, please don't mix code hardening fixes with unrelated behaviour changes.
What do you mean? Which parts of the code changes do I need splitting? |
AFAICS, "Meter: Prevent integer overflow on x positions" is the actual overflow fix, while the other two commits change behaviour. Or did I miss something? |
"CPUMeter: Fix negative "x" positions of sub-meters" is related and necessary after the first fix because this fixes one of the conditions that x can wrap to a negative number. Since the first patch adds assertions about Perhaps I can reorder the patch so that this "CPUMeter" patch is applied first, before the main one ("Meter: Prevent integer overflow on x positions"), to reduce confusion. The third patch, "Meter: Don't draw caption or borders if width is not enough", is to further remove potential undefined behaviours by strengthen the logics. It does change behaviours as you said, but I believe I cannot split this one out as the original logic could involve in UB in certain conditions. In other words, this is related anyway. |
A formula error in CPUMeterCommonDraw() can cause some CPU sub-meters to have negative "x" positions before drawing. (The negative "x" can happen when the terminal screen width is very small.) Fix the formula of calculating "colwidth" in the function. After this fix, the "x" value can no longer be negative but the "colwidth" value (the terminal width of a sub-meter column) can now be zero. (Regression from commit 7fdd8d3) Signed-off-by: Kang-Che Sung <explorer09@gmail.com>
* Add assertions to "x" and "w" values in the meter's draw() functions. The caller should ensure that (x + w) never overflows a signed int type. * In BarMeterMode_draw(), the "offset + blockSizes[i]" calculation might also overflow. Cap the maximum of "blockSizes[i]" rather than cap the result of the addition. * In LEDMeterMode_draw(), adjust the loop break conditions (on "xx" variable) so that the loop can terminate properly with "xx" values very close to the INT_MAX limit. Signed-off-by: Kang-Che Sung <explorer09@gmail.com>
* Don't call mvaddnstr() when the calculated length to print is 0 or
negative.
* If the width ("w" value) for a meter is not enough to print more than
its caption, let the draw() functions reset the color attributes and
return early. Prevent a potential undefined behaviour where
(x + captionLen) can overflow.
Signed-off-by: Kang-Che Sung <explorer09@gmail.com>
If the width given to a bar or graph meter is not enough to draw a caption, skip printing the caption. In this case the meter will now draw nothing at all.
Explorer09
force-pushed
the
meter-integer-overflow
branch
from
7ff6631 to
89bf83a
Compare
2 participants
xandwparameters in the meter'sdraw()functions to prevent(x + w)overflowing a signed int type. Also add assertions toxandwin the debug build.(x >= 0 && w <= INT_MAX - x)BarMeterMode_draw(), theoffset + blockSizes[i]calculation might also overflow. Cap the maximum ofblockSizes[i]rather than cap the result of the addition.LEDMeterMode_draw(), adjust the loop break conditions (onxxvariable) so that the loop can terminate properly withxxvalues very close to theINT_MAXlimit.