From c6ce48ef190d74b11c265076518dcc69f6fe4665 Mon Sep 17 00:00:00 2001
From: Eric Miao <ericymiao@google.com>
Date: Fri, 14 Jul 2023 16:35:09 -0700
Subject: [PATCH] String8: fix infinite loop and segmentation fault in
 removeAll()

Bug: 290835996
Test: libutils_fuzz_string8 for several minutes

String8::removeAll() has 2 serious problems:

1. When `other` is an empty string, `removeAll()` will loop infinitely
   due to below process:

   a) with `other` being empty string `""`, find() will call strstr()
      on an empty string, which always returns `mString`, and thus
      find() always return 0 in this case
   b) with find() returns 0 for empty string, the next while loop in
      String8::removeAll() will keep loop infinitely as `index` will
      always be 0

   This CL fixes this problem by returning true if `other` is an empty
   string (i.e. `strlen(other) == 0`), this follows the logic that an
   empty string will always be found and no actual remove needs to be
   done.

2. When `other` is a NULL string, strstr() has undefined behavior. See
   https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf.

   This undefined behavior on Android unfortunately causes immediate
   segmentation fault as the current `strstr` implementation in bionic
   libc doesn't check `needle` being NULL, and an access to a NULL
   location is performed to check if the `needle` string is an empty
   string, and thus causes segmentation fault.

   This CL gives an error message and aborts instead of having a
   segfault, and to keep some backward compatibility.

   This CL also adds test for String8::removeAll()

Change-Id: Ie2ccee6767efe0fed476db4ec6072717198279e9
---
 libutils/String8.cpp      |  5 +++++
 libutils/String8_test.cpp | 18 ++++++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/libutils/String8.cpp b/libutils/String8.cpp
index 82f5cb682..8d312b57c 100644
--- a/libutils/String8.cpp
+++ b/libutils/String8.cpp
@@ -393,6 +393,11 @@ ssize_t String8::find(const char* other, size_t start) const
 }
 
 bool String8::removeAll(const char* other) {
+    ALOG_ASSERT(other, "String8::removeAll() requires a non-NULL string");
+
+    if (*other == '\0')
+        return true;
+
     ssize_t index = find(other);
     if (index < 0) return false;
 
diff --git a/libutils/String8_test.cpp b/libutils/String8_test.cpp
index 1356cd08f..35fd512f4 100644
--- a/libutils/String8_test.cpp
+++ b/libutils/String8_test.cpp
@@ -114,3 +114,21 @@ TEST_F(String8Test, append) {
     EXPECT_EQ(NO_MEMORY, s.append("baz", SIZE_MAX));
     EXPECT_STREQ("foobar", s);
 }
+
+TEST_F(String8Test, removeAll) {
+    String8 s("Hello, world!");
+
+    // NULL input should cause an assertion failure and error message in logcat
+    EXPECT_DEATH(s.removeAll(NULL), "");
+
+    // expect to return true and string content should remain unchanged
+    EXPECT_TRUE(s.removeAll(""));
+    EXPECT_STREQ("Hello, world!", s);
+
+    // expect to return false
+    EXPECT_FALSE(s.removeAll("x"));
+    EXPECT_STREQ("Hello, world!", s);
+
+    EXPECT_TRUE(s.removeAll("o"));
+    EXPECT_STREQ("Hell, wrld!", s);
+}