Menu
▾
▴
#6468 Possible buffer overflow bug in VectorRendererSpec.cpp
Ver 1.6.0
Possible buffer overflow bug found in
void VectorRendererSpec<pixeltype>::drawBevelSquareAlg</pixeltype>
1286 w = MIN(w + (bevel * 2), (int)_activeSurface->w);
1287 h = MIN(h + (bevel * 2), (int)_activeSurface->h);
Can allow for items to be drawn outside the pixel buffer.
Suggested fix:
1286 w = MIN(x + w + (bevel * 2), (int)_activeSurface->w);
1287 h = MIN(y + h + (bevel * 2), (int)_activeSurface->h);
Steps to reproduce:
Switch to builtin theme on 1x scaling.
Open the options dialog.
Set a breakpoint on drawBevelSquareAlg
Open the render mode dropdown and break on drawBevelSquareAlg
Dropdown tries to draw row 201 on 320x200 canvas. Overflow occurs on line 1300 when i == 0
Actually the suggested fix is also wrong ignore that. Having another look just now.
Corrected fix:
1286 w = MIN(x + w + (bevel * 2), (int)_activeSurface->w) - x;
1287 h = MIN(y + h + (bevel * 2), (int)_activeSurface->h) - y;
Gareth: Thank you for this code review... However, I have tried debugging the replication case you indicated with Valgrind under Linux and I can't see any bad/out of bound memory accesses occuring here.
Reviewing your code description, can you be clear at the bug case, what the values are for the arguments of drawBevelSquareAlg(int x, int y, int w, int h, int bevel, PixelType top_color, PixelType bottom_color, bool fill) at entry to the function and the values of _activeSurface->w and h ...
Just that the 0 case should never happen as I think the while should exit prior to this point... though I am wondering if this is a prefix / postfix error on the decrement...
Gareth: This could also be a bug with GUI code which positions the options dialog on the screen i.e. off by one error for calculating the position... though again I can't replicate this.
Could you also please try this with the latest v1.7.0git development master code from our github page to ensure that you are not chasing a "ghost" caused by a bug already fixed...
It's indeed pretty clear that the current clipping code is broken in drawBevelSquareAlg. Your (second) suggested fix looks good at first glance, but I haven't tried or tested it yet.
I'm out of town the weekend but ill gather the requested info once I'm back and validate against v1.7.0.
in essence though the bug allows you to paint outside the bounds of the screen since it does not take into account the items x or y coordinate when determining the maximum width or height. The 0 case does run but I don't think that's a prob. if the width or height is within the screen bounds it will never overpaint.
I agree on your point about calculating positions. I think there is a secondary more intricate bug since the item dimensions should all be precalculated to fit but it adds the bevel back onto the outside causing the overall item dimensions to be larger than its original width.