Opened 11 years ago

Closed 7 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

Ticket imported from: bugs/6468.

Attachments (1)

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

Download all attachments as: .zip

Change History (13)

comment:1 by SF/garethbp, 11 years ago

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

by SF/garethbp, 11 years ago

Attachment: Screenshot.png added

comment:2 by SF/garethbp, 11 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 digitall, 11 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 digitall, 11 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 wjp, 11 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 SF/garethbp, 11 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 SF/garethbp, 11 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 csnover, 7 years ago

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 by csnover, 7 years ago

Owner: set to csnover

comment:10 by csnover, 7 years ago

Keywords: has-patch added

comment:11 by csnover, 7 years ago

Component: GUI
Keywords: reproducible added

comment:12 by csnover, 7 years ago

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.