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

[Marc Mutz]

On Tuesday 15 March 2016 14:44:12 Rutledge Shawn wrote:
> > On 15 Mar 2016, at 15:43, Marc Mutz <marc.mutz at kdab.com> wrote:
> > 
> > On Tuesday 15 March 2016 13:08:42 Bo Thorsen wrote:
> >> Den 15-03-2016 kl. 14:07 skrev Marc Mutz:
> > [...]
> > 
> >> There is another option that doesn't mean a change of signature: Bound
> >> the result. So if the real result is > INT_MAX, return INT_MAX. Same for
> >> INT_MIN.
> >> 
> >> Yes, it's not the correct result, but I completely agree with you that
> >> it's a theoretical problem. As long as it's documented in the width() I
> >> really don't see the problem with this solution.
> > 
> > I like the idea to change width() to return a bounded result to avoid UB
> > for old users, but we need a code path that returns the correct result
> > for new users without everyone of them going quint64(1) + r.right() -
> > r.left() by themselves…
> 
> 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).

> 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(). 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 (which 
https://codereview.qt-project.org/152354 should already get rid of, i.e that 
isNull() returns true in such a situation).

But it's also about designing an API that's easy to use correctly and hard to 
use incorrectly. Making width() return a sufficiently large type would make it 
hard to use QRect incorrectly. We can do that for Qt 6, but we need a solution 
for Qt 5, too. Thus, whatever clutter is introduced by this change, it will be 
gone in Qt 6 (except for an inline forwarder), so it's not worse than any 
other example of deprecating API in favour of a new one.

Thanks,
Marc

-- 
Marc Mutz <marc.mutz at kdab.com> | Senior Software Engineer
KDAB (Deutschland) GmbH & Co.KG, a KDAB Group Company
Tel: +49-30-521325470
KDAB - The Qt Experts