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

13 ↗

(On Diff #174042)

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

13 ↗

(On Diff #174042)

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

13 ↗

(On Diff #174042)

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 ↗

(On Diff #176930)

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

13 ↗

(On Diff #174042)

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

13 ↗

(On Diff #174042)

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 ↗

(On Diff #176930)

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 ↗

(On Diff #176930)

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

[vitalybuka]

lib/sanitizer_common/sanitizer_common_interceptors.inc

7728 ↗

(On Diff #176930)

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 ↗

(On Diff #176930)

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 ↗

(On Diff #177104)

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

44 ↗

(On Diff #177104)

asserts?

[krytarowski]

lib/sanitizer_common/sanitizer_common_interceptors.inc

7735 ↗

(On Diff #177104)

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

test/sanitizer_common/TestCases/NetBSD/strtonum.cc

44 ↗

(On Diff #177104)

I will switch to asserts.

[vitalybuka]

lib/sanitizer_common/sanitizer_common_interceptors.inc

7735 ↗

(On Diff #177104)

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

compiler-rt/trunk/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;
+  COMMON_INTERCEPTOR_ENTER(ctx, strtonum, nptr, minval, maxval, errstr);
+
+  // TODO(kamil): Implement strtoll as a common inteceptor
+  char *real_endptr;
+  long long ret = (long long)REAL(strtoimax)(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;
+}
+#define INIT_STRTONUM COMMON_INTERCEPT_FUNCTION(strtonum)
+#else
+#define INIT_STRTONUM
+#endif
+
 static void InitializeCommonInterceptors() {
   static u64 metadata_mem[sizeof(MetadataHashMap) / sizeof(u64) + 1];
   interceptor_metadata_map = new((void *)&metadata_mem) MetadataHashMap();
 
   INIT_MMAP;
   INIT_MMAP64;
   INIT_TEXTDOMAIN;
   INIT_STRLEN;
static void InitializeCommonInterceptors() {
   INIT_GETVFSSTAT;
   INIT_REGEX;
   INIT_FTS;
   INIT_SYSCTL;
   INIT_ASYSCTL;
   INIT_SYSCTLGETMIBINFO;
   INIT_NL_LANGINFO;
   INIT_MODCTL;
+  INIT_STRTONUM;
 
   INIT___PRINTF_CHK;
 }

compiler-rt/trunk/lib/sanitizer_common/sanitizer_platform_interceptors.h

 #define SANITIZER_INTERCEPT_GETVFSSTAT SI_NETBSD
 #define SANITIZER_INTERCEPT_REGEX SI_NETBSD
 #define SANITIZER_INTERCEPT_FTS SI_NETBSD
 #define SANITIZER_INTERCEPT_SYSCTL (SI_NETBSD || SI_FREEBSD)
 #define SANITIZER_INTERCEPT_ASYSCTL SI_NETBSD
 #define SANITIZER_INTERCEPT_SYSCTLGETMIBINFO SI_NETBSD
 #define SANITIZER_INTERCEPT_NL_LANGINFO (SI_NETBSD || SI_FREEBSD)
 #define SANITIZER_INTERCEPT_MODCTL SI_NETBSD
+#define SANITIZER_INTERCEPT_STRTONUM SI_NETBSD
 
 #endif  // #ifndef SANITIZER_PLATFORM_INTERCEPTORS_H

compiler-rt/trunk/test/sanitizer_common/TestCases/NetBSD/strtonum.cc

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