[libc] fix strtol returning the wrong length

[michaelrj]

Summary

Previously, strtol/ll/ul/ull would return a pointer to the end of its
parsing, regardless of if it detected a number. Now it will return a
length of 0 when it doesn't find a number.

[sivachandra]

libc/src/__support/str_conv_utils.h

65

Nit: s/seen_digit/is_number?

83

Do we need this if block at all?

[michaelrj]

add tests for lone prefixes

libc/src/__support/str_conv_utils.h

83

it's to handle the case where the number is just "0" and the base is set to automatic. Without this, then that would be parsed as an octal number with no digits, as opposed to a decimal number with one digit. I've added a test to check this.
The other part of the condition was unnecessary though.

[sivachandra]

libc/src/__support/str_conv_utils.h

83

May be infer_base is incorrect then? Consider this:

const char *n = "08";
char *next;
long i = ::strtol(n, &next, 0);

Since "08" is an invalid number, should we want the above strtolcall to succeed or a fail? If it is a failure, should next point to 8 or 0? Should it be the same if the base was explicitly specified to be 8?

I also think we can ask the same questions when base is 16.

My reading of the standard says that "08" is an invalid number so no conversion should be performed. Which means, str_end should point to original_src. Likewise, "0xZ" with base 16 is an invalid number so no conversion should be performed and str_end should point to original_src.

[michaelrj]

fix infer_base to not advance the pointer unnecessarily

[michaelrj]

libc/src/__support/str_conv_utils.h

83

infer_base was incorrect, and I have now fixed it so that it doesn't skip over the leading octal 0, and removed this condition.

From what I can tell, in the case of both "08" and "0xZ", when parsed with an automatic base, what we have is a solo octal 0 (and this matches what I see from other libc implementations).

In the C standard §6.4.4.1 it specifies that "A decimal constant begins with a nonzero digit", meaning that those numbers cannot be assumed to be base 10, but that "An octal constant consists of the prefix 0 optionally followed by a sequence of the digits 0 through 7 only". This means that the longest valid interpretation of "08" is that it is an octal 0 followed by some value that is not a valid digit (equivalent to "0Z").

For "0xZ" it is similar, a hexadecimal number has to have a valid hexadecimal digit after the "0x" so this is just 0.

[sivachandra]

libc/src/__support/str_conv_utils.h

45

Replace

This will advance the string pointer.

with

This function will advance |src| to the first valid digit in the inferred base.

83

Ah, yes!

I am reading two different parts of the standard in isolation. 6.4.4.1 Has this text:

An octal constant consists of the prefix 0 optionally followed by a sequence of the
digits 0 through 7 only. A hexadecimal constant consists of the prefix 0x or 0X followed
by a sequence of the decimal digits and the letters a (or A) through f (or F) with values
10 through 15 respectively.

Which makes 08 an invalid literal. But, the spec for strto<int> functions also says:

decompose the input string into three parts: an initial, possibly empty, sequence of
white-space characters (as specified by the isspace function), a subject sequence
resembling an integer represented in some radix determined by the value of base, and a
final string of one or more unrecognized characters, including the terminating null
character of the input string.

So, in the string "08", '8' is the first unrecognized character, so the string consists of only one valid base 8 character "0". Combining the above two parts of the standard, "08" should be interpreted as just the octal zero. Which is what you are saying I think.

This same argument does not hold for "0x" or "0xZ" because just the prefix 0x or the number 0xZ are not hex numbers at all. But, the octal argument kicks in here as well: so "0x" is octal zero, and "0xZ" is also octal zero. I just checked what other libcs do and it looks like glibc atleast follows this argument. In both the cases, the end_ptr is updated by only 1 implying that only one valid character was read.

If you are convinced with this, can you update infer_base to reflect all of this. Also add comments quoting the appropriate sections in the standard.

[michaelrj]

update comments to reflect the reasoning behing infer_base

libc/src/__support/str_conv_utils.h

 // checks if the next 3 characters of the string pointer are the start of a
 // hexadecimal number. Does not advance the string pointer.
 static inline bool is_hex_start(const char *__restrict src) {
   return *src == '0' && (*(src + 1) | 32) == 'x' && isalnum(*(src + 2)) &&
          b36_char_to_int(*(src + 2)) < 16;
 }
 
 // Takes the address of the string pointer and parses the base from the start of
 // it. This will advance the string pointer.
 static inline int infer_base(const char *__restrict *__restrict src) {
   if (is_hex_start(*src)) {
     (*src) += 2;
     return 16;
   } else if (**src == '0') {
     ++(*src);
     return 8;
   } else {
     return 10;
   }
 }
 
 // Takes a pointer to a string, a pointer to a string pointer, and the base to
 // convert to. This function is used as the backend for all of the string to int
 // functions.
 template <class T>
 static inline T strtointeger(const char *__restrict src,
                              char **__restrict str_end, int base) {
   unsigned long long result = 0;
+  bool seen_digit = false;
+  const char *original_src = src;
 
   if (base < 0 || base == 1 || base > 36) {
     errno = EINVAL; // NOLINT
     return 0;
   }
 
   src = first_non_whitespace(src);
 
   char result_sign = '+';
   if (*src == '+' || *src == '-') {
     result_sign = *src;
     ++src;
   }
 
+  if (*src == '0' || (isalnum(*src) && b36_char_to_int(*src) < base)) {
+    seen_digit = true;
+  }
+
   if (base == 0) {
     base = infer_base(&src);
   } else if (base == 16 && is_hex_start(src)) {
     src = src + 2;
   }
 
   constexpr bool is_unsigned = (__llvm_libc::cpp::NumericLimits<T>::min() == 0);
   const bool is_positive = (result_sign == '+');
   unsigned long long constexpr NEGATIVE_MAX =
       !is_unsigned ? static_cast<unsigned long long>(
                          __llvm_libc::cpp::NumericLimits<T>::max()) +
                          1
                    : __llvm_libc::cpp::NumericLimits<T>::max();
   unsigned long long const ABS_MAX =
       (is_positive ? __llvm_libc::cpp::NumericLimits<T>::max() : NEGATIVE_MAX);
   unsigned long long const ABS_MAX_DIV_BY_BASE = ABS_MAX / base;
   while (isalnum(*src)) {
     int cur_digit = b36_char_to_int(*src);
     if (cur_digit >= base)
       break;
 
+    seen_digit = true;
     ++src;
 
     // If the number has already hit the maximum value for the current type then
     // the result cannot change, but we still need to advance src to the end of
     // the number.
     if (result == ABS_MAX) {
       errno = ERANGE; // NOLINT
       continue;
if (result > ABS_MAX - cur_digit) {
       result = ABS_MAX;
       errno = ERANGE; // NOLINT
     } else {
       result = result + cur_digit;
     }
   }
 
   if (str_end != nullptr)
-    *str_end = const_cast<char *>(src);
+    *str_end = const_cast<char *>(seen_digit ? src : original_src);
 
   if (result == ABS_MAX) {
     if (is_positive || is_unsigned)
       return __llvm_libc::cpp::NumericLimits<T>::max();
     else // T is signed and there is a negative overflow
       return __llvm_libc::cpp::NumericLimits<T>::min();
   }
 
   return is_positive ? static_cast<T>(result) : -static_cast<T>(result);
 }
 
 } // namespace internal
 } // namespace __llvm_libc
 
 #endif // LIBC_SRC_STDLIB_STDLIB_UTILS_H

libc/test/src/stdlib/strtol_test.cpp

TEST(LlvmLibcStrToLTest, MessyBaseTenDecode) {
   ASSERT_EQ(__llvm_libc::strtol(two_numbers, &str_end, 10), 10l);
   ASSERT_EQ(errno, 0);
   EXPECT_EQ(str_end - two_numbers, ptrdiff_t(2));
 
   const char *two_signs = "--10 999";
   errno = 0;
   ASSERT_EQ(__llvm_libc::strtol(two_signs, &str_end, 10), 0l);
   ASSERT_EQ(errno, 0);
-  EXPECT_EQ(str_end - two_signs, ptrdiff_t(1));
+  EXPECT_EQ(str_end - two_signs, ptrdiff_t(0));
 
   const char *sign_before = "+2=4";
   errno = 0;
   ASSERT_EQ(__llvm_libc::strtol(sign_before, &str_end, 10), 2l);
   ASSERT_EQ(errno, 0);
   EXPECT_EQ(str_end - sign_before, ptrdiff_t(2));
 
   const char *sign_after = "2+2=4";
   errno = 0;
   ASSERT_EQ(__llvm_libc::strtol(sign_after, &str_end, 10), 2l);
   ASSERT_EQ(errno, 0);
   EXPECT_EQ(str_end - sign_after, ptrdiff_t(1));
 
   const char *tab_before = "\t10";
   errno = 0;
   ASSERT_EQ(__llvm_libc::strtol(tab_before, &str_end, 10), 10l);
   ASSERT_EQ(errno, 0);
   EXPECT_EQ(str_end - tab_before, ptrdiff_t(3));
 
   const char *all_together = "\t  -12345and+67890";
   errno = 0;
   ASSERT_EQ(__llvm_libc::strtol(all_together, &str_end, 10), -12345l);
   ASSERT_EQ(errno, 0);
   EXPECT_EQ(str_end - all_together, ptrdiff_t(9));
+
+  const char *just_spaces = "  ";
+  errno = 0;
+  ASSERT_EQ(__llvm_libc::strtol(just_spaces, &str_end, 10), 0l);
+  ASSERT_EQ(errno, 0);
+  EXPECT_EQ(str_end - just_spaces, ptrdiff_t(0));
+
+  const char *just_space_and_sign = " +";
+  errno = 0;
+  ASSERT_EQ(__llvm_libc::strtol(just_space_and_sign, &str_end, 10), 0l);
+  ASSERT_EQ(errno, 0);
+  EXPECT_EQ(str_end - just_space_and_sign, ptrdiff_t(0));
 }
 
 static char int_to_b36_char(int input) {
   if (input < 0 || input > 36)
     return '0';
   if (input < 10)
     return static_cast<char>('0' + input);
   return static_cast<char>('A' + input - 10);

libc/test/src/stdlib/strtoll_test.cpp

TEST(LlvmLibcStrToLLTest, MessyBaseTenDecode) {
   ASSERT_EQ(__llvm_libc::strtoll(two_numbers, &str_end, 10), 10ll);
   ASSERT_EQ(errno, 0);
   EXPECT_EQ(str_end - two_numbers, ptrdiff_t(2));
 
   const char *two_signs = "--10 999";
   errno = 0;
   ASSERT_EQ(__llvm_libc::strtoll(two_signs, &str_end, 10), 0ll);
   ASSERT_EQ(errno, 0);
-  EXPECT_EQ(str_end - two_signs, ptrdiff_t(1));
+  EXPECT_EQ(str_end - two_signs, ptrdiff_t(0));
 
   const char *sign_before = "+2=4";
   errno = 0;
   ASSERT_EQ(__llvm_libc::strtoll(sign_before, &str_end, 10), 2ll);
   ASSERT_EQ(errno, 0);
   EXPECT_EQ(str_end - sign_before, ptrdiff_t(2));
 
   const char *sign_after = "2+2=4";
   errno = 0;
   ASSERT_EQ(__llvm_libc::strtoll(sign_after, &str_end, 10), 2ll);
   ASSERT_EQ(errno, 0);
   EXPECT_EQ(str_end - sign_after, ptrdiff_t(1));
 
   const char *tab_before = "\t10";
   errno = 0;
   ASSERT_EQ(__llvm_libc::strtoll(tab_before, &str_end, 10), 10ll);
   ASSERT_EQ(errno, 0);
   EXPECT_EQ(str_end - tab_before, ptrdiff_t(3));
 
   const char *all_together = "\t  -12345and+67890";
   errno = 0;
   ASSERT_EQ(__llvm_libc::strtoll(all_together, &str_end, 10), -12345ll);
   ASSERT_EQ(errno, 0);
   EXPECT_EQ(str_end - all_together, ptrdiff_t(9));
+
+  const char *just_spaces = "  ";
+  errno = 0;
+  ASSERT_EQ(__llvm_libc::strtoll(just_spaces, &str_end, 10), 0ll);
+  ASSERT_EQ(errno, 0);
+  EXPECT_EQ(str_end - just_spaces, ptrdiff_t(0));
+
+  const char *just_space_and_sign = " +";
+  errno = 0;
+  ASSERT_EQ(__llvm_libc::strtoll(just_space_and_sign, &str_end, 10), 0ll);
+  ASSERT_EQ(errno, 0);
+  EXPECT_EQ(str_end - just_space_and_sign, ptrdiff_t(0));
 }
 
 static char int_to_b36_char(int input) {
   if (input < 0 || input > 36)
     return '0';
   if (input < 10)
     return static_cast<char>('0' + input);
   return static_cast<char>('A' + input - 10);

libc/test/src/stdlib/strtoul_test.cpp

TEST(LlvmLibcStrToULTest, MessyBaseTenDecode) {
   ASSERT_EQ(__llvm_libc::strtoul(two_numbers, &str_end, 10), 10ul);
   ASSERT_EQ(errno, 0);
   EXPECT_EQ(str_end - two_numbers, ptrdiff_t(2));
 
   const char *two_signs = "--10 999";
   errno = 0;
   ASSERT_EQ(__llvm_libc::strtoul(two_signs, &str_end, 10), 0ul);
   ASSERT_EQ(errno, 0);
-  EXPECT_EQ(str_end - two_signs, ptrdiff_t(1));
+  EXPECT_EQ(str_end - two_signs, ptrdiff_t(0));
 
   const char *sign_before = "+2=4";
   errno = 0;
   ASSERT_EQ(__llvm_libc::strtoul(sign_before, &str_end, 10), 2ul);
   ASSERT_EQ(errno, 0);
   EXPECT_EQ(str_end - sign_before, ptrdiff_t(2));
 
   const char *sign_after = "2+2=4";
   errno = 0;
   ASSERT_EQ(__llvm_libc::strtoul(sign_after, &str_end, 10), 2ul);
   ASSERT_EQ(errno, 0);
   EXPECT_EQ(str_end - sign_after, ptrdiff_t(1));
 
   const char *tab_before = "\t10";
   errno = 0;
   ASSERT_EQ(__llvm_libc::strtoul(tab_before, &str_end, 10), 10ul);
   ASSERT_EQ(errno, 0);
   EXPECT_EQ(str_end - tab_before, ptrdiff_t(3));
 
   const char *all_together = "\t  -12345and+67890";
   errno = 0;
   ASSERT_EQ(__llvm_libc::strtoul(all_together, &str_end, 10), -(12345ul));
   ASSERT_EQ(errno, 0);
   EXPECT_EQ(str_end - all_together, ptrdiff_t(9));
+
+  const char *just_spaces = "  ";
+  errno = 0;
+  ASSERT_EQ(__llvm_libc::strtoul(just_spaces, &str_end, 10), 0ul);
+  ASSERT_EQ(errno, 0);
+  EXPECT_EQ(str_end - just_spaces, ptrdiff_t(0));
+
+  const char *just_space_and_sign = " +";
+  errno = 0;
+  ASSERT_EQ(__llvm_libc::strtoul(just_space_and_sign, &str_end, 10), 0ul);
+  ASSERT_EQ(errno, 0);
+  EXPECT_EQ(str_end - just_space_and_sign, ptrdiff_t(0));
 }
 
 static char int_to_b36_char(int input) {
   if (input < 0 || input > 36)
     return '0';
   if (input < 10)
     return static_cast<char>('0' + input);
   return static_cast<char>('A' + input - 10);

libc/test/src/stdlib/strtoull_test.cpp

TEST(LlvmLibcStrToULLTest, MessyBaseTenDecode) {
   ASSERT_EQ(__llvm_libc::strtoull(two_numbers, &str_end, 10), 10ull);
   ASSERT_EQ(errno, 0);
   EXPECT_EQ(str_end - two_numbers, ptrdiff_t(2));
 
   const char *two_signs = "--10 999";
   errno = 0;
   ASSERT_EQ(__llvm_libc::strtoull(two_signs, &str_end, 10), 0ull);
   ASSERT_EQ(errno, 0);
-  EXPECT_EQ(str_end - two_signs, ptrdiff_t(1));
+  EXPECT_EQ(str_end - two_signs, ptrdiff_t(0));
 
   const char *sign_before = "+2=4";
   errno = 0;
   ASSERT_EQ(__llvm_libc::strtoull(sign_before, &str_end, 10), 2ull);
   ASSERT_EQ(errno, 0);
   EXPECT_EQ(str_end - sign_before, ptrdiff_t(2));
 
   const char *sign_after = "2+2=4";
   errno = 0;
   ASSERT_EQ(__llvm_libc::strtoull(sign_after, &str_end, 10), 2ull);
   ASSERT_EQ(errno, 0);
   EXPECT_EQ(str_end - sign_after, ptrdiff_t(1));
 
   const char *tab_before = "\t10";
   errno = 0;
   ASSERT_EQ(__llvm_libc::strtoull(tab_before, &str_end, 10), 10ull);
   ASSERT_EQ(errno, 0);
   EXPECT_EQ(str_end - tab_before, ptrdiff_t(3));
 
   const char *all_together = "\t  -12345and+67890";
   errno = 0;
   ASSERT_EQ(__llvm_libc::strtoull(all_together, &str_end, 10), -(12345ull));
   ASSERT_EQ(errno, 0);
   EXPECT_EQ(str_end - all_together, ptrdiff_t(9));
+
+  const char *just_spaces = "  ";
+  errno = 0;
+  ASSERT_EQ(__llvm_libc::strtoull(just_spaces, &str_end, 10), 0ull);
+  ASSERT_EQ(errno, 0);
+  EXPECT_EQ(str_end - just_spaces, ptrdiff_t(0));
+
+  const char *just_space_and_sign = " +";
+  errno = 0;
+  ASSERT_EQ(__llvm_libc::strtoull(just_space_and_sign, &str_end, 10), 0ull);
+  ASSERT_EQ(errno, 0);
+  EXPECT_EQ(str_end - just_space_and_sign, ptrdiff_t(0));
 }
 
 static char int_to_b36_char(int input) {
   if (input < 0 || input > 36)
     return '0';
   if (input < 10)
     return static_cast<char>('0' + input);
   return static_cast<char>('A' + input - 10);