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

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

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

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

7268

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

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

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

7266–7287

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

7266–7267

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

[vitalybuka]

lib/sanitizer_common/sanitizer_common_interceptors.inc

7266–7267

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

7266–7287

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

7275

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

7275

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

7275

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

 }
 #define INIT_NETENT \
   COMMON_INTERCEPT_FUNCTION(getnetent); \
   COMMON_INTERCEPT_FUNCTION(getnetbyname); \
   COMMON_INTERCEPT_FUNCTION(getnetbyaddr)
 #else
 #define INIT_NETENT
 #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);
+  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);
+  }
+  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;
   INIT_STRNLEN;
static void InitializeCommonInterceptors() {
   INIT_STRLCPY;
   INIT_DEVNAME;
   INIT_DEVNAME_R;
   INIT_FGETLN;
   INIT_STRMODE;
   INIT_TTYENT;
   INIT_PROTOENT;
   INIT_NETENT;
+  INIT_STRTONUM;
 
   INIT___PRINTF_CHK;
 }

lib/sanitizer_common/sanitizer_platform_interceptors.h

 
 #define SANITIZER_INTERCEPT_DEVNAME (SI_NETBSD || SI_OPENBSD || SI_FREEBSD)
 #define SANITIZER_INTERCEPT_DEVNAME_R (SI_NETBSD || SI_FREEBSD)
 #define SANITIZER_INTERCEPT_FGETLN (SI_NETBSD || SI_FREEBSD)
 #define SANITIZER_INTERCEPT_STRMODE (SI_NETBSD || SI_FREEBSD)
 #define SANITIZER_INTERCEPT_TTYENT SI_NETBSD
 #define SANITIZER_INTERCEPT_PROTOENT SI_NETBSD
 #define SANITIZER_INTERCEPT_NETENT SI_NETBSD
+#define SANITIZER_INTERCEPT_STRTONUM SI_NETBSD
 
 #endif  // #ifndef SANITIZER_PLATFORM_INTERCEPTORS_H

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>
+#include <err.h>
+
+int main(void) {
+  long long l;
+  const char *errstr;
+
+  l = strtonum("100", 1, 100, &errstr);
+  if (errstr)
+    errx(1, "strtonum: %s", errstr);
+  printf("%lld\n", l);
+
+  l = strtonum("200", 1, 100, &errstr);
+  if (!errstr)
+    errx(1, "strtonum");
+  printf("%s\n", errstr);
+
+  l = strtonum("300", 1000, 1001, &errstr);
+  if (!errstr)
+    errx(1, "strtonum");
+  printf("%s\n", errstr);
+
+  l = strtonum("abc", 1000, 1001, &errstr);
+  if (!errstr)
+    errx(1, "strtonum");
+  printf("%s\n", errstr);
+
+  // CHECK: 100
+  // CHECK: too large
+  // CHECK: too small
+  // CHECK: invalid
+
+  return 0;
+}