Opened 5 years ago

Closed 19 months ago

#6468 closed defect (fixed)

Possible buffer overflow bug in VectorRendererSpec.cpp

Reported by: SF/garethbp Owned by: csnover
Priority: blocker Component: GUI
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)

Screenshot.png (109.5 KB) - added by SF/garethbp 5 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 5 years ago by SF/garethbp

Actually the suggested fix is also wrong ignore that. Having another look just now.

Changed 5 years ago by SF/garethbp

Attachment: Screenshot.png added

comment:2 Changed 5 years ago by SF/garethbp

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 Changed 5 years ago by digitall

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 Changed 5 years ago by digitall

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 Changed 5 years ago by wjp

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 Changed 5 years ago by SF/garethbp

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 Changed 5 years ago by SF/garethbp

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 Changed 19 months ago by csnover

Priority: normalblocker

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 Changed 19 months ago by csnover

Owner: set to csnover

comment:10 Changed 19 months ago by csnover

Keywords: has-patch added

comment:11 Changed 19 months ago by csnover

Component: GUI
Keywords: reproducible added

comment:12 Changed 19 months ago by csnover

Resolution: fixed
Status: newclosed

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.

Note: See TracTickets for help on using tickets.