From 4acdb60ba38cf0c579d9b630d55868eb0571d724 Mon Sep 17 00:00:00 2001
From: Ben Wiederhake <BenWiederhake.GitHub@gmx.de>
Date: Sat, 22 Aug 2020 16:10:19 +0200
Subject: [PATCH] AK: Prevent confusing silent misuse of ByteBuffer

Thankfully, this hasn't happened in any other code yet, but it happened
while I was trying something out. Using '==' on two ByteBuffers to check
whether they're equal seemed straight-forward, so I ran into the trap.
---
 AK/ByteBuffer.cpp           | 52 +++++++++++++++++++++++++++++++++++++
 AK/ByteBuffer.h             |  8 ++++++
 AK/Tests/TestByteBuffer.cpp | 24 ++++++++++-------
 3 files changed, 74 insertions(+), 10 deletions(-)
 create mode 100644 AK/ByteBuffer.cpp

diff --git a/AK/ByteBuffer.cpp b/AK/ByteBuffer.cpp
new file mode 100644
index 00000000000..9e9af19c6d1
--- /dev/null
+++ b/AK/ByteBuffer.cpp
@@ -0,0 +1,52 @@
+/*
+ * Copyright (c) 2020, the SerenityOS developers.
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are met:
+ *
+ * 1. Redistributions of source code must retain the above copyright notice, this
+ *    list of conditions and the following disclaimer.
+ *
+ * 2. Redistributions in binary form must reproduce the above copyright notice,
+ *    this list of conditions and the following disclaimer in the documentation
+ *    and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+ * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE
+ * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
+ * SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
+ * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+ * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include <AK/ByteBuffer.h>
+
+namespace AK {
+
+bool ByteBuffer::operator==(const ByteBuffer& other) const
+{
+    if (is_empty() != other.is_empty())
+        return false;
+    if (is_empty())
+        return true;
+    if (size() != other.size())
+        return false;
+
+    // So they both have data, and the same length.
+    // Avoid hitting conditionals in ever iteration.
+    size_t n = size();
+    const u8* this_data = data();
+    const u8* other_data = other.data();
+    for (size_t i = 0; i < n; ++i) {
+        if (this_data[i] != other_data[i])
+            return false;
+    }
+    return true;
+}
+
+}
diff --git a/AK/ByteBuffer.h b/AK/ByteBuffer.h
index 2c53a578fa4..43a79c2df18 100644
--- a/AK/ByteBuffer.h
+++ b/AK/ByteBuffer.h
@@ -147,6 +147,14 @@ public:
     bool operator!() const { return is_null(); }
     bool is_null() const { return m_impl == nullptr; }
 
+    // Disable default implementations that would use surprising integer promotion.
+    bool operator==(const ByteBuffer& other) const;
+    bool operator!=(const ByteBuffer& other) const { return !(*this == other); }
+    bool operator<=(const ByteBuffer& other) const = delete;
+    bool operator>=(const ByteBuffer& other) const = delete;
+    bool operator<(const ByteBuffer& other) const = delete;
+    bool operator>(const ByteBuffer& other) const = delete;
+
     u8& operator[](size_t i)
     {
         ASSERT(m_impl);
diff --git a/AK/Tests/TestByteBuffer.cpp b/AK/Tests/TestByteBuffer.cpp
index ce2e323336d..b813829cb5e 100644
--- a/AK/Tests/TestByteBuffer.cpp
+++ b/AK/Tests/TestByteBuffer.cpp
@@ -53,17 +53,21 @@ TEST_CASE(equality_operator)
     EXPECT_EQ(d == d, true);
 }
 
-TEST_CASE(other_operators)
+/*
+ * FIXME: These `negative_*` tests should cause precisely one compilation error
+ * each, and always for the specified reason. Currently we do not have a harness
+ * for that, so in order to run the test you need to set the #define to 1, compile
+ * it, and check the error messages manually.
+ */
+#define COMPILE_NEGATIVE_TESTS 0
+#if COMPILE_NEGATIVE_TESTS
+TEST_CASE(negative_operator_lt)
 {
-    ByteBuffer a = ByteBuffer::copy("Hello, world", 7);
-    ByteBuffer b = ByteBuffer::copy("Hello, friend", 7);
-    // `a` and `b` are both "Hello, ".
-    ByteBuffer c = ByteBuffer::copy("asdf", 4);
-    ByteBuffer d;
-    EXPECT_EQ(a < a, true);
-    EXPECT_EQ(a <= b, true);
-    EXPECT_EQ(a >= c, false);
-    EXPECT_EQ(a > d, false);
+    ByteBuffer a = ByteBuffer::copy("Hello, world", 10);
+    ByteBuffer b = ByteBuffer::copy("Hello, friend", 10);
+    (void)(a < b);
+    // error: error: use of deleted function ‘bool AK::ByteBuffer::operator<(const AK::ByteBuffer&) const’
 }
+#endif /* COMPILE_NEGATIVE_TESTS */
 
 TEST_MAIN(ByteBuffer)