[libc] Fix strtok_r crash when src and *saveptr are both nullptr

[gAlfonso-bit]

This comment was removed by gAlfonso-bit.

[gAlfonso-bit]

This comment was removed by gAlfonso-bit.

[gAlfonso-bit]

This comment was removed by gAlfonso-bit.

[sivachandra]

Couple of nits inline. I can submit this for you after you update.

libc/src/string/string_utils.h

79

Can we avoid this pattern and do it in two steps:

*src = '\0';
src++;

libc/test/src/string/strtok_r_test.cpp

89

May be add:

ASSERT_STREQ(reserver, nullptr);

May be that is what your intention was below, but not sure.

[gAlfonso-bit]

This comment was removed by gAlfonso-bit.

[gAlfonso-bit]

This comment was removed by gAlfonso-bit.

[gAlfonso-bit]

This comment was removed by gAlfonso-bit.

[gAlfonso-bit]

libc/test/src/string/strtok_r_test.cpp

89

Yeah that is what I meant. Thank you!

[sivachandra]

Will submit shortly.

[gAlfonso-bit]

This comment was removed by gAlfonso-bit.

[gAlfonso-bit]

This comment was removed by gAlfonso-bit.

[gAlfonso-bit]

This comment was removed by gAlfonso-bit.

[sivachandra]

I need your name and email address to attribute this change to you.

[gAlfonso-bit]

This comment was removed by gAlfonso-bit.

libc/src/string/string_utils.h

 //===-- String utils --------------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LIBC_SRC_STRING_STRING_UTILS_H
 #define LIBC_SRC_STRING_STRING_UTILS_H
 
+#include "src/__support/common.h"
 #include "utils/CPP/Bitset.h"
 #include <stddef.h> // size_t
 
 namespace __llvm_libc {
 namespace internal {
 
 // Returns the length of a string, denoted by the first occurrence
 // of a null terminator.

 // found. We define this span as a token. The end of the token is appended with
 // a null terminator, and the token is returned. The point where the last token
 // is found is then stored within 'context' for subsequent calls. Subsequent
 // calls will use 'context' when a nullptr is passed in for 'src'. Once the null
 // terminating character is reached, returns a nullptr.
 static inline char *string_token(char *__restrict src,
                                  const char *__restrict delimiter_string,
                                  char **__restrict saveptr) {
+  // Return nullptr immediately if both src AND saveptr are nullptr
+  if (unlikely(src == nullptr && ((src = *saveptr) == nullptr)))
+    return nullptr;
+
   cpp::Bitset<256> delimiter_set;
-  for (; *delimiter_string; ++delimiter_string)
+  for (; *delimiter_string != '\0'; ++delimiter_string)
     delimiter_set.set(*delimiter_string);
 
-  src = src ? src : *saveptr;
-  for (; *src && delimiter_set.test(*src); ++src)
+  for (; *src != '\0' && delimiter_set.test(*src); ++src)
     ;
-  if (!*src) {
+  if (*src == '\0') {
     *saveptr = src;
     return nullptr;
   }
   char *token = src;
-  for (; *src && !delimiter_set.test(*src); ++src)
-    ;
-  if (*src) {
+  for (; *src != '\0'; ++src) {
+    if (delimiter_set.test(*src)) {
       *src = '\0';
       ++src;
+      break;
+    }
   }
   *saveptr = src;
   return token;
 }
 
 } // namespace internal
 } // namespace __llvm_libc
 
 #endif //  LIBC_SRC_STRING_STRING_UTILS_H

libc/test/src/string/strtok_r_test.cpp

TEST(LlvmLibcStrTokReentrantTest, ShouldNotGoPastNullTerminator) {
   char *reserve = nullptr;
   ASSERT_STREQ(__llvm_libc::strtok_r(src, ",", &reserve), "12");
   // Another call to ensure that 'reserve' is not in a bad state.
   ASSERT_STREQ(__llvm_libc::strtok_r(src, ",", &reserve), "12");
   ASSERT_STREQ(__llvm_libc::strtok_r(nullptr, ",", &reserve), nullptr);
 }
 
 TEST(LlvmLibcStrTokReentrantTest,
+     ShouldReturnNullptrWhenBothSrcAndSaveptrAreNull) {
+  char *src = nullptr;
+  char *reserve = nullptr;
+  // Ensure that instead of crashing if src and reserve are null, nullptr is
+  // returned
+  ASSERT_STREQ(__llvm_libc::strtok_r(src, ",", &reserve), nullptr);
+  // And that neither src nor reserve are changed when that happens
+  ASSERT_STREQ(src, nullptr);
+  ASSERT_STREQ(reserve, nullptr);
+}
+
+TEST(LlvmLibcStrTokReentrantTest,
      SubsequentCallsShouldFindFollowingDelimiters) {
   char src[] = "12,34.56";
   char *reserve = nullptr;
   char *token = __llvm_libc::strtok_r(src, ",.", &reserve);
   ASSERT_STREQ(token, "12");
   token = __llvm_libc::strtok_r(nullptr, ",.", &reserve);
   ASSERT_STREQ(token, "34");
   token = __llvm_libc::strtok_r(nullptr, ",.", &reserve);