Commit c6ce48ef authored Jul 14, 2023 by Eric Miao

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

parent cb199b47

libutils/String8.cpp

+5 −0

Original line number	Diff line number	Diff line
		@@ -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;

libutils/String8_test.cpp

+18 −0

Original line number	Diff line number	Diff line
		@@ -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);
		}