[Development] Fixing QRect::width() / height()

[Poenitz Andre]

Marc Mutz <marc.mutz at kdab.com> wrote:
> > Cluttering up the API doesn’t seem nice.  Also not sure what you mean by
> > new users needing such large rectangles… if they do, why don’t they use
> > QRectF?
> 
> For one, QRectF has different semantics w.r.t. wdith()/height(), for another,
> sizeof(QRectF) == 2 * sizeof(QRect).

That does not mean that the needed semantics would not be achievable.

> > Or is it about a security hole?
>
> Yes, that, too. If you read a rectangle from user input, then an attacker can
> currently force you to construct a QRect with overflowing bounds. ATM, calling
> width() on this QRect invokes UB. Bo's idea would fix the UB, but return
> something other than the width(). 

The return value would be the same before, except for values around INT_MAX.
Such values are *not* expected in a legitimate use of QRect ("Screen coordinates"),
so a behaviour change there will not affect normal users. 

The clamping achieves exactly what is wanted: It plug the hole without 
side-effects for supported uses.

> It's not impossible that this could be
> exploited, too, e.g. if the app divides by a non-isNull rect's width and the
> attacker forges the coordinates such that width() return 0 

Please describe how that can happen. 

width() return will only change around INT_MAX, not around 0.

> But it's also about designing an API that's easy to use correctly and hard to
> use incorrectly. 

Adding a second width() version is exactly that: It designs an API that's
harder to use ("which version do I need"?), leaves all existing users
in the rain, and would trigger major changes in code that wants to adapt.

Clamping width() fixes the problem for existing user code automatically,
and leaves the API unambiguous.

Andre'