From 26bef6ec2e6478c764638bd1a0016471d3212b63 Mon Sep 17 00:00:00 2001
From: Marc Mutz <marc.mutz@qt.io>
Date: Tue, 17 Jan 2023 14:09:22 +0100
Subject: [PATCH] QHttp1Configuration: fix UB (inactive union member access)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Any QHttp1Configuration object created has 'ShortData data' as active
member. std::exchange()ing or qt_ptr_swap()ping 'Private *d', then, is
accessing the inactive member of the union, and that's UB.

Fix by swapping and copying the whole union, as opposed to any of its
members, and, in the move constructor, activating Private *d.

This is now safe, as assigning to 'd' ends 'data's life-time and
starts 'd's. Even if we assign a well-formed object to a moved-from
object, we either swap or copy the whole union, so SEP. For
self-move-assignment in the moved-from state (Hinnant Criterion),
we're using std::swap() on the whole union, so SEP.

In addition, activating `Private *d` in moved-from objects means that
a future ~Public's use of unconditional 'delete d' won't invoke UB,
either.

Thanks to Peppe for insisting on fixing this.

Change-Id: Ic1323b8416d6b17ae21768c625de1daba0944133
Reviewed-by: Mårten Nordheim <marten.nordheim@qt.io>
(cherry picked from commit 47abdabe2cfd20715b0186b9c257bc32db9315bb)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>
---
 src/network/access/qhttp1configuration.cpp | 10 +++++-----
 src/network/access/qhttp1configuration.h   |  9 +++++----
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/src/network/access/qhttp1configuration.cpp b/src/network/access/qhttp1configuration.cpp
index 2e804d66e41..cfa929bca50 100644
--- a/src/network/access/qhttp1configuration.cpp
+++ b/src/network/access/qhttp1configuration.cpp
@@ -36,7 +36,7 @@ static_assert(sizeof(QHttp1Configuration) == sizeof(void*),
     Default constructs a QHttp1Configuration object.
 */
 QHttp1Configuration::QHttp1Configuration()
-    : data{6, {}} // QHttpNetworkConnectionPrivate::defaultHttpChannelCount
+    : u(ShortData{6, {}}) // QHttpNetworkConnectionPrivate::defaultHttpChannelCount
 {
 }
 
@@ -91,7 +91,7 @@ void QHttp1Configuration::setNumberOfConnectionsPerHost(qsizetype number)
     auto n = qt_saturate<std::uint8_t>(number);
     if (n == 0)
         return;
-    data.numConnectionsPerHost = n;
+    u.data.numConnectionsPerHost = n;
 }
 
 /*!
@@ -102,7 +102,7 @@ void QHttp1Configuration::setNumberOfConnectionsPerHost(qsizetype number)
 */
 qsizetype QHttp1Configuration::numberOfConnectionsPerHost() const
 {
-    return data.numConnectionsPerHost;
+    return u.data.numConnectionsPerHost;
 }
 
 /*!
@@ -140,7 +140,7 @@ qsizetype QHttp1Configuration::numberOfConnectionsPerHost() const
 */
 bool QHttp1Configuration::equals(const QHttp1Configuration &other) const noexcept
 {
-    return data.numConnectionsPerHost == other.data.numConnectionsPerHost;
+    return u.data.numConnectionsPerHost == other.u.data.numConnectionsPerHost;
 }
 
 /*!
@@ -148,7 +148,7 @@ bool QHttp1Configuration::equals(const QHttp1Configuration &other) const noexcep
 */
 size_t QHttp1Configuration::hash(size_t seed) const noexcept
 {
-    return qHash(data.numConnectionsPerHost, seed);
+    return qHash(u.data.numConnectionsPerHost, seed);
 }
 
 QT_END_NAMESPACE
diff --git a/src/network/access/qhttp1configuration.h b/src/network/access/qhttp1configuration.h
index 91e4554256c..4b66745f54a 100644
--- a/src/network/access/qhttp1configuration.h
+++ b/src/network/access/qhttp1configuration.h
@@ -22,7 +22,7 @@ public:
     Q_NETWORK_EXPORT QHttp1Configuration();
     Q_NETWORK_EXPORT QHttp1Configuration(const QHttp1Configuration &other);
     QHttp1Configuration(QHttp1Configuration &&other) noexcept
-        : d{std::exchange(other.d, nullptr)} {}
+        : u{other.u} { other.u.d = nullptr; }
 
     Q_NETWORK_EXPORT QHttp1Configuration &operator=(const QHttp1Configuration &other);
     QT_MOVE_ASSIGNMENT_OPERATOR_IMPL_VIA_PURE_SWAP(QHttp1Configuration)
@@ -33,17 +33,18 @@ public:
     Q_NETWORK_EXPORT qsizetype numberOfConnectionsPerHost() const;
 
     void swap(QHttp1Configuration &other) noexcept
-    { qt_ptr_swap(d, other.d); }
+    { std::swap(u, other.u); }
 
 private:
     struct ShortData {
         std::uint8_t numConnectionsPerHost;
         char reserved[sizeof(void*) - sizeof(numConnectionsPerHost)];
     };
-    union {
+    union U {
+        U(ShortData _data) : data(_data) {}
         QHttp1ConfigurationPrivate *d;
         ShortData data;
-    };
+    } u;
 
     Q_NETWORK_EXPORT bool equals(const QHttp1Configuration &other) const noexcept;
     Q_NETWORK_EXPORT size_t hash(size_t seed) const noexcept;