From 2d648a8efdf57467451cd32666a3ed5a594deb5a Mon Sep 17 00:00:00 2001 From: Marc Mutz Date: Tue, 12 Dec 2023 20:51:56 +0100 Subject: [PATCH 1/2] HPack: fix a Yoda Condition Putting the variable on the LHS of a relational operation makes the expression easier to read. In this case, we find that the whole expression is nonsensical as an overflow protection, because if name.size() + value.size() overflows, the result will exactly _not_ be > max() - 32, because UB will have happened. To be fixed in a follow-up commit. As a drive-by, add parentheses around the RHS. Pick-to: 6.2 5.15 Change-Id: I35ce598884c37c51b74756b3bd2734b9aad63c09 Reviewed-by: Allan Sandfeld Jensen (cherry picked from commit 658607a34ead214fbacbc2cca44915655c318ea9) Reviewed-by: Qt Cherry-pick Bot (cherry picked from commit 4f7efd41740107f90960116700e3134f5e433867) (cherry picked from commit 13c16b756900fe524f6d9534e8a07aa003c05e0c) --- src/network/access/http2/hpacktable.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/network/access/http2/hpacktable.cpp b/src/network/access/http2/hpacktable.cpp index 74a09a207ff..c8c5d098c80 100644 --- a/src/network/access/http2/hpacktable.cpp +++ b/src/network/access/http2/hpacktable.cpp @@ -27,7 +27,7 @@ HeaderSize entry_size(QByteArrayView name, QByteArrayView value) // 32 octets of overhead." const unsigned sum = unsigned(name.size() + value.size()); - if (std::numeric_limits::max() - 32 < sum) + if (sum > (std::numeric_limits::max() - 32)) return HeaderSize(); return HeaderSize(true, quint32(sum + 32)); } From 76a65aa5f26724d69e5e50a758300df546a4370c Mon Sep 17 00:00:00 2001 From: Marc Mutz Date: Tue, 12 Dec 2023 22:08:07 +0100 Subject: [PATCH 2/2] HPack: fix incorrect integer overflow check This code never worked: For the comparison with max() - 32 to trigger, on 32-bit platforms (or Qt 5) signed interger overflow would have had to happen in the addition of the two sizes. The compiler can therefore remove the overflow check as dead code. On Qt 6 and 64-bit platforms, the signed integer addition would be very unlikely to overflow, but the following truncation to uint32 would yield the correct result only in a narrow 32-value window just below UINT_MAX, if even that. Fix by using the proper tool, qAddOverflow. Pick-to: 6.2 5.15 Change-Id: I7599f2e75ff7f488077b0c60b81022591005661c Reviewed-by: Allan Sandfeld Jensen (cherry picked from commit ee5da1f2eaf8932aeca02ffea6e4c618585e29e3) Reviewed-by: Qt Cherry-pick Bot (cherry picked from commit debeb8878da2dc706ead04b6072ecbe7e5313860) Reviewed-by: Thiago Macieira Reviewed-by: Marc Mutz (cherry picked from commit 811b9eef6d08d929af8708adbf2a5effb0eb62d7) --- src/network/access/http2/hpacktable.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/network/access/http2/hpacktable.cpp b/src/network/access/http2/hpacktable.cpp index c8c5d098c80..2c728b37e3b 100644 --- a/src/network/access/http2/hpacktable.cpp +++ b/src/network/access/http2/hpacktable.cpp @@ -26,7 +26,9 @@ HeaderSize entry_size(QByteArrayView name, QByteArrayView value) // for counting the number of references to the name and value would have // 32 octets of overhead." - const unsigned sum = unsigned(name.size() + value.size()); + size_t sum; + if (qAddOverflow(size_t(name.size()), size_t(value.size()), &sum)) + return HeaderSize(); if (sum > (std::numeric_limits::max() - 32)) return HeaderSize(); return HeaderSize(true, quint32(sum + 32));