Opened 10 years ago
Closed 6 years ago
#6468 closed defect (fixed)
Possible buffer overflow bug in VectorRendererSpec.cpp
| Reported by: | SF/garethbp | Owned by: | csnover |
|---|---|---|---|
| Priority: | blocker | Component: | GUI |
| Version: | Keywords: | has-patch reproducible | |
| Cc: | Game: |
Description
Ver 1.6.0
Possible buffer overflow bug found in
void VectorRendererSpec::drawBevelSquareAlg
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
Ticket imported from: bugs/6468.
Attachments (1)
Change History (13)
comment:1 by , 10 years ago
by , 10 years ago
| Attachment: | Screenshot.png added |
|---|
comment:2 by , 10 years ago
Corrected fix:
1286 w = MIN(x + w + (bevel * 2), (int)_activeSurface->w) - x; 1287 h = MIN(y + h + (bevel * 2), (int)_activeSurface->h) - y;
comment:3 by , 10 years ago
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...
comment:4 by , 10 years ago
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...
comment:5 by , 10 years ago
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.
comment:6 by , 10 years ago
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.
comment:7 by , 10 years ago
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.
comment:8 by , 6 years ago
| Priority: | normal → blocker |
|---|
Raising all identified crasher, hang, and memory violation bugs which I could not fully triage myself to blocker priority for the next release.
comment:9 by , 6 years ago
| Owner: | set to |
|---|
comment:10 by , 6 years ago
| Keywords: | has-patch added |
|---|
comment:11 by , 6 years ago
| Component: | → GUI |
|---|---|
| Keywords: | reproducible added |
comment:12 by , 6 years ago
| Resolution: | → fixed |
|---|---|
| Status: | new → closed |
Thanks for your report! A patch for this issue has been added in commit 0d8afad55980eee2f3e634680f93d08c580a5364 and will be available in daily builds 1.10.0git-5361 and later.
Actually the suggested fix is also wrong ignore that. Having another look just now.