On Tuesday 15 March 2016 14:44:12 Rutledge Shawn wrote: > > On 15 Mar 2016, at 15:43, Marc Mutz <[email protected]> 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 <[email protected]> | Senior Software Engineer KDAB (Deutschland) GmbH & Co.KG, a KDAB Group Company Tel: +49-30-521325470 KDAB - The Qt Experts _______________________________________________ Development mailing list [email protected] http://lists.qt-project.org/mailman/listinfo/development
