From 17eddf3ac4cc334a989bfd1f1f36c47439d5d18c Mon Sep 17 00:00:00 2001
From: Max Wipfli <mail@maxwipfli.ch>
Date: Thu, 1 Jul 2021 17:52:20 +0200
Subject: [PATCH] AK: Add input bounds checking to String::substring()

This checks for overflow in String::substring(). It also rearranges some
declarations in the header.
---
 AK/String.cpp | 22 +++++++++++-----------
 AK/String.h   |  5 ++---
 2 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/AK/String.cpp b/AK/String.cpp
index 66e29ef77d7..d4b49615b17 100644
--- a/AK/String.cpp
+++ b/AK/String.cpp
@@ -91,6 +91,16 @@ String String::isolated_copy() const
     return String(move(*impl));
 }
 
+String String::substring(size_t start, size_t length) const
+{
+    if (!length)
+        return String::empty();
+    VERIFY(m_impl);
+    VERIFY(!Checked<size_t>::addition_would_overflow(start, length));
+    VERIFY(start + length <= m_impl->length());
+    return { characters() + start, length };
+}
+
 String String::substring(size_t start) const
 {
     VERIFY(m_impl);
@@ -98,21 +108,11 @@ String String::substring(size_t start) const
     return { characters() + start, length() - start };
 }
 
-String String::substring(size_t start, size_t length) const
-{
-    if (!length)
-        return "";
-    VERIFY(m_impl);
-    VERIFY(start + length <= m_impl->length());
-    // FIXME: This needs some input bounds checking.
-    return { characters() + start, length };
-}
-
 StringView String::substring_view(size_t start, size_t length) const
 {
     VERIFY(m_impl);
+    VERIFY(!Checked<size_t>::addition_would_overflow(start, length));
     VERIFY(start + length <= m_impl->length());
-    // FIXME: This needs some input bounds checking.
     return { characters() + start, length };
 }
 
diff --git a/AK/String.h b/AK/String.h
index 94100f376e1..89a906d87f6 100644
--- a/AK/String.h
+++ b/AK/String.h
@@ -140,6 +140,7 @@ public:
 
     [[nodiscard]] Vector<String> split_limit(char separator, size_t limit, bool keep_empty = false) const;
     [[nodiscard]] Vector<String> split(char separator, bool keep_empty = false) const;
+    [[nodiscard]] Vector<StringView> split_view(char separator, bool keep_empty = false) const;
 
     [[nodiscard]] Optional<size_t> find(char needle, size_t start = 0) const { return StringUtils::find(*this, needle, start); }
     [[nodiscard]] Optional<size_t> find(StringView const& needle, size_t start = 0) const { return StringUtils::find(*this, needle, start); }
@@ -147,10 +148,8 @@ public:
     // FIXME: Implement find_last(StringView const&) for API symmetry.
     [[nodiscard]] Vector<size_t> find_all(StringView const& needle) const { return StringUtils::find_all(*this, needle); }
 
-    [[nodiscard]] String substring(size_t start) const;
     [[nodiscard]] String substring(size_t start, size_t length) const;
-
-    [[nodiscard]] Vector<StringView> split_view(char separator, bool keep_empty = false) const;
+    [[nodiscard]] String substring(size_t start) const;
     [[nodiscard]] StringView substring_view(size_t start, size_t length) const;
     [[nodiscard]] StringView substring_view(size_t start) const;