Add new interceptor for strtonum(3)

[krytarowski]

Summary

strtonum(3) reliably convertss string value to an integer.
This function is used in OpenBSD compat namespace
and is located inside NetBSD's libc.

Add a dedicated test for this interface.

It's a reworked version of the original code by Yang Zheng.

[vitalybuka]

test/sanitizer_common/TestCases/NetBSD/strtonum.cc

14

what is going to happen with following?
strtonum("100 long suffix....", 0, 1000, &errstr)
if this stops parsing just after 100 then COMMON_INTERCEPTOR_READ_RANGE(ctx, nptr, REAL(strlen)(nptr) + 1) is performance bottleneck
maybe we should get actual read size with StrtolFixAndCheck

we had issues with other strto* with strict_string_checks=1 on parsers like python
it just calls such methods in the middle of a large file and you get O(N) -> O(N^2)

[vitalybuka]

test/sanitizer_common/TestCases/NetBSD/strtonum.cc

14

maybe we should get actual read size with StrtolFixAndCheck

correction: get actual read size with different strto* call and pass it into StrtolFixAndCheck

[krytarowski]

• rework the strtonum(3) interceptor according to comments from review

[krytarowski]

test/sanitizer_common/TestCases/NetBSD/strtonum.cc

14

strtonum("100 long suffix....", 0, 1000, &errstr) it will stop parsin after 100 on ' '.

src.illumos.org/source/xref/openbsd-src/lib/libc/stdlib/strtonum.c

[krytarowski]

lib/sanitizer_common/sanitizer_common_interceptors.inc

7732

I had to define on top these variables, as compiler due to goto was complaining that the code is invalid otherwise.

[krytarowski]

If this patch looks fine, I will follow up with this approach in strtoi(3)/strtou(3) https://reviews.llvm.org/D54702

[vitalybuka]

test/sanitizer_common/TestCases/NetBSD/strtonum.cc

14

According implementation form src.illumos.org/source/xref/openbsd-src/lib/libc/stdlib/strtonum.c
it uses strtoll
So if we intercept strtoll, we just need to unpoison errstrp, it it was in existing implementation. What is the problem this patch is solving?

[krytarowski]

test/sanitizer_common/TestCases/NetBSD/strtonum.cc

14

Do you mean what the interceptor code is solving? Reading input string only as much as the strtonum(3) function and unpoisioning errstr.

[vitalybuka]

lib/sanitizer_common/sanitizer_common_interceptors.inc

7742

INTERCEPTOR(long long, strtonum, const char *nptr, long long minval,
            long long maxval, const char **errstr) {
  void *ctx;
  COMMON_INTERCEPTOR_ENTER(ctx, strtonum, nptr, minval, maxval, errstr);
  
  char *real_endptr;
  long long ret = REAL(strtoll)(nptr, &real_endptr, 10);
  StrtolFixAndCheck(ctx, nptr, nullptr, real_endptr, 10);

  ret = REAL(strtonum)(nptr, minval, maxval, errstr);
  if (errstr) {
    COMMON_INTERCEPTOR_WRITE_RANGE(ctx, errstr, sizeof(const char *));
     if (*errstr)
      COMMON_INTERCEPTOR_WRITE_RANGE(ctx, *errstr, REAL(strlen)(*errstr) + 1);
  }
  return ret;
}

[vitalybuka]

lib/sanitizer_common/sanitizer_common_interceptors.inc

7728

Patch says that base already has SANITIZER_INTERCEPT_STRTONUM
but master has non of it

[vitalybuka]

lib/sanitizer_common/sanitizer_common_interceptors.inc

7728

Phabricator does something weird:
Base vs Diff 1 : it's a new interceptor
Base vs Diff 2 : it's modified interceptor

I guess I was confused by this, I assumed that you are changing interceptor, not adding new one.
So in Diff 1 I don't like REAL(strlen). So I proposed to use something to get actual read len and use StrtolFixAndCheck.

I think it's better to just to call parsing twice (line in snippets I've sent), instead of exposing error handling into interceptor, like in Diff 2.

However I am not sure what is going to happen on BSD when we call REAL(strtonum). Is this going to hit strtoll interceptor, which is unfortunately not in common yet?

[krytarowski]

Hmm, I will try to reupload diff. It's a new interceptor.

[krytarowski]

• eliminate goto
• fix diff

[krytarowski]

lib/sanitizer_common/sanitizer_common_interceptors.inc

7742

Hmm.. this is evaluating the input string twice... but performance impact is probably marginal, so I will switch to it.

[krytarowski]

• add a new version

[vitalybuka]

lib/sanitizer_common/sanitizer_common_interceptors.inc

7735

why not strtoll, it returns the same type as interceptor and it's already used in strtonum implementation?

test/sanitizer_common/TestCases/NetBSD/strtonum.cc

45

asserts?

[krytarowski]

lib/sanitizer_common/sanitizer_common_interceptors.inc

7735

It doesn't build, as strtoll isn't a valid interceptor for all sanitizers.

test/sanitizer_common/TestCases/NetBSD/strtonum.cc

45

I will switch to asserts.

[vitalybuka]

lib/sanitizer_common/sanitizer_common_interceptors.inc

7735

Ah right, to call REAL we need interceptor or some workaround.
LGTM as is, but we should move these from asan/msan to common at some point

lib/sanitizer_common/sanitizer_common_interceptors.inc

INTERCEPTOR(int, modctl, int operation, void *argp) {
   return ret;
 }
 #define INIT_MODCTL COMMON_INTERCEPT_FUNCTION(modctl)
 #else
 #define INIT_MODCTL
 #endif
 
 #if SANITIZER_INTERCEPT_STRTONUM
 INTERCEPTOR(long long, strtonum, const char *nptr, long long minval,
             long long maxval, const char **errstr) {
   void *ctx;
+  INTMAX_T res;
+  int saved_errno;
   COMMON_INTERCEPTOR_ENTER(ctx, strtonum, nptr, minval, maxval, errstr);
-  if (nptr)
-    COMMON_INTERCEPTOR_READ_RANGE(ctx, nptr, REAL(strlen)(nptr) + 1);
-  long long ret = REAL(strtonum)(nptr, minval, maxval, errstr);
-  if (errstr) {
-    COMMON_INTERCEPTOR_WRITE_RANGE(ctx, errstr, sizeof(const char *));
-    if (*errstr)
-      COMMON_INTERCEPTOR_WRITE_RANGE(ctx, *errstr, REAL(strlen)(*errstr) + 1);
+  const char *rerrstr;
+  if (!errstr)
+    errstr = &rerrstr;
+
+  if (minval > maxval) {
+    *errstr = "invalid";
+    goto skip;
+  }
+
+  saved_errno = errno;
+  errno = 0;
+
+  char *real_endptr;
+  res = REAL(strtoimax)(nptr, &real_endptr, 10);
+  StrtolFixAndCheck(ctx, nptr, nullptr, real_endptr, 10);
+  if (nptr == real_endptr || *real_endptr != '\0')
+    *errstr = "invalid";
+  else if ((res == INTMAX_MAX && errno == errno_ERANGE) || (maxval < res))
+    *errstr = "too large";
+  else if ((res == INTMAX_MIN && errno == errno_ERANGE) || (minval > res))
+    *errstr = "too small";
+  else
+    *errstr = nullptr;
+
+  errno = saved_errno;
+
+ skip:
+  COMMON_INTERCEPTOR_WRITE_RANGE(ctx, errstr, sizeof(*errstr));
+  if (*errstr) {
+    res = 0;
   }
-  return ret;
+
+  return res;
 }
 #define INIT_STRTONUM COMMON_INTERCEPT_FUNCTION(strtonum)
 #else
 #define INIT_STRTONUM
 #endif
 
 static void InitializeCommonInterceptors() {
   static u64 metadata_mem[sizeof(MetadataHashMap) / sizeof(u64) + 1];

lib/sanitizer_common/sanitizer_errno_codes.h

 #ifndef SANITIZER_ERRNO_CODES_H
 #define SANITIZER_ERRNO_CODES_H
 
 namespace __sanitizer {
 
 #define errno_ENOMEM 12
 #define errno_EBUSY 16
 #define errno_EINVAL 22
+#define errno_ERANGE 34
 
 // Those might not present or their value differ on different platforms.
 extern const int errno_EOWNERDEAD;
 
 }  // namespace __sanitizer
 
 #endif  // SANITIZER_ERRNO_CODES_H

lib/sanitizer_common/sanitizer_internal_defs.h

 #undef UINT32_MAX
 #define UINT32_MAX             (4294967295U)
 #undef INT64_MIN
 #define INT64_MIN              (-__INT64_C(9223372036854775807)-1)
 #undef INT64_MAX
 #define INT64_MAX              (__INT64_C(9223372036854775807))
 #undef UINT64_MAX
 #define UINT64_MAX             (__UINT64_C(18446744073709551615))
+#undef INTMAX_MIN
+#define INTMAX_MIN             INT64_MIN
+#undef INTMAX_MAX
+#define INTMAX_MAX             INT64_MAX
 #undef UINTPTR_MAX
 #if SANITIZER_WORDSIZE == 64
 # define UINTPTR_MAX           (18446744073709551615UL)
 #else
 # define UINTPTR_MAX           (4294967295U)
 #endif  // SANITIZER_WORDSIZE == 64
 
 enum LinkerInitialized { LINKER_INITIALIZED = 0 };

test/sanitizer_common/TestCases/NetBSD/strtonum.cc

+// RUN: %clangxx -O0 -g %s -o %t && %run %t 2>&1 | FileCheck %s
+
+#define _OPENBSD_SOURCE
+
+#include <stdio.h>
+#include <stdlib.h>
+
+int main(void) {
+  const char *errstr;
+
+  printf("strtonum\n");
+
+  long long l = strtonum("100", 1, 100, &errstr);
+  if (errstr)
+    abort();
+  printf("%lld\n", l);
+
+  l = strtonum("200", 1, 100, &errstr);
+  if (!errstr)
+    abort();
+  printf("%s\n", errstr);
+
+  l = strtonum("300", 1000, 1001, &errstr);
+  if (!errstr)
+    abort();
+  printf("%s\n", errstr);
+
+  l = strtonum("abc", 1000, 1001, &errstr);
+  if (!errstr)
+    abort();
+  printf("%s\n", errstr);
+
+  l = strtonum("1000", 1001, 1000, &errstr);
+  if (!errstr)
+    abort();
+  printf("%s\n", errstr);
+
+  l = strtonum("1000abc", 1000, 1001, &errstr);
+  if (!errstr)
+    abort();
+  printf("%s\n", errstr);
+
+  l = strtonum("1000.0", 1000, 1001, &errstr);
+  if (!errstr)
+    abort();
+  printf("%s\n", errstr);
+
+  // CHECK: strtonum
+  // CHECK: 100
+  // CHECK: too large
+  // CHECK: too small
+  // CHECK: invalid
+  // CHECK: invalid
+  // CHECK: invalid
+  // CHECK: invalid
+
+  return 0;
+}