[sanitizer] use uptr to store the result of internal_strlen/internal_strnlen in printf_common

[Enna1]

Summary

The return type of internal_strlen() is 'uptr', but in printf_common() we store the result of internal_strlen() into an 'int' type variable.
When the result value of internal_strlen() is larger than the largest possible value of 'int' type, the implicit conversion from 'uptr' to 'int' will change the result value to a negative value.

Without this change, asan reports a false positive negative-size-param in the added testcase.

[Enna1]

format

[Enna1]

Fix Wsign-compare

[fmayer]

compiler-rt/test/asan/TestCases/vsnprintf.cpp

5 ↗

(On Diff #547717)

can we just do {{.*windows}}? that would match the comment

[fmayer]

compiler-rt/test/asan/TestCases/vsnprintf.cpp

5 ↗

(On Diff #547717)

{{.windows.*}}, of course.

[Enna1]

update from @fmayer comment

[Enna1]

compiler-rt/test/asan/TestCases/vsnprintf.cpp

5 ↗

(On Diff #547717)

Thanks!
This line was copied from test/asan/printf-2.c
updated to {{.*windows.*}}

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors_format.inc

569

COMMON_INTERCEPTOR_READ_RANGE(ctx, argp, 0);

compiler-rt/test/asan/TestCases/vsnprintf.cpp

20 ↗

(On Diff #548093)

does this pass on 32bit platforms?

[vitalybuka]

compiler-rt/test/asan/TestCases/vsnprintf.cpp

2 ↗

(On Diff #548093)

the patch is for sanitizer_common/
so the test should be test/sanitizer_common/

[vitalybuka]

compiler-rt/test/asan/TestCases/vsnprintf.cpp

20 ↗

(On Diff #548093)

******************** TEST 'AddressSanitizer-i386-linux-dynamic :: TestCases/vsnprintf.cpp' FAILED ********************
Script:
--
: 'RUN: at line 1';      out/./bin/clang  --driver-mode=g++ -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only  -m32  -shared-libasan -O2 llvm-project/compiler-rt/test/asan/TestCases/vsnprintf.cpp -o out/projects/compiler-rt/test/asan/I386LinuxDynamicConfig/TestCases/Output/vsnprintf.cpp.tmp
: 'RUN: at line 2';   env ASAN_OPTIONS=check_printf=1  out/projects/compiler-rt/test/asan/I386LinuxDynamicConfig/TestCases/Output/vsnprintf.cpp.tmp 2>&1
--
Exit Code: 134

Command Output (stdout):
--
terminate called after throwing an instance of 'std::length_error'
  what():  basic_string::_M_create

--
Command Output (stderr):
--
out/projects/compiler-rt/test/asan/I386LinuxDynamicConfig/TestCases/Output/vsnprintf.cpp.script: line 2: 2820733 Aborted                 env ASAN_OPTIONS=check_printf=1 out/projects/compiler-rt/test/asan/I386LinuxDynamicConfig/TestCases/Output/vsnprintf.cpp.tmp 2>&1

--

********************
********************
Failed Tests (2):
  AddressSanitizer-i386-linux :: TestCases/vsnprintf.cpp
  AddressSanitizer-i386-linux-dynamic :: TestCases/vsnprintf.cpp

[Enna1]

Thanks for testing.

I see in compiler-rt/test/sanitizer_common/TestCases/allocator_returns_null.cpp, for 64-bit kMaxAllowedMallocSize is 1ULL<<40, for 32-bit kMaxAllowedMallocSize is 3UL << 30.
So I update this test, use malloc(1UL<<31) instead of std::string, I think this can pass on 32-bit platforms.
But I don't know how to test build and test 32-bit sanitizer, so I'm not sure about it.

Could you please describe how to build and test 32-bit sanitizer? Thanks!

[vitalybuka]

In D157266#4598359, @Enna1 wrote:

Thanks for testing.

I see in compiler-rt/test/sanitizer_common/TestCases/allocator_returns_null.cpp, for 64-bit kMaxAllowedMallocSize is 1ULL<<40, for 32-bit kMaxAllowedMallocSize is 3UL << 30.
So I update this test, use malloc(1UL<<31) instead of std::string, I think this can pass on 32-bit platforms.

Probably it's fine to disable the test on 32bit platforms.

But I don't know how to test build and test 32-bit sanitizer, so I'm not sure about it.

Could you please describe how to build and test 32-bit sanitizer? Thanks!

That's how it's enabled on bots, and probably on my workstation. https://github.com/google/sanitizers/blob/26afa5f62cef5079e8cdfe96972fdfa057ed1b4b/buildbot/install_deps.sh#L16

Then cmake should automatically add them into check-compiler-rt check-asan etc
You should see in the cmake log:

• Compiler-RT supported architectures: x86_64;i386

[Enna1]

Thanks! I have run the test on 32-bit platforms successfully.

Update test to run this test only on x86_64-target-arch.

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors_format.inc

if (size == FSS_INVALID) {
       break;
     }
     if (dir.convSpecifier == 'n') {
       void *argp = va_arg(aq, void *);
       COMMON_INTERCEPTOR_WRITE_RANGE(ctx, argp, size);
       continue;
     } else if (size == FSS_STRLEN) {
       if (void *argp = va_arg(aq, void *)) {
+        uptr len;
         if (dir.starredPrecision) {
           // FIXME: properly support starred precision for strings.
-          size = 0;
+          len = 0;
         } else if (dir.fieldPrecision > 0) {
           // Won't read more than "precision" symbols.
-          size = internal_strnlen((const char *)argp, dir.fieldPrecision);
-          if (size < dir.fieldPrecision) size++;
+          len = internal_strnlen((const char *)argp, dir.fieldPrecision);
+          if (len < (uptr)dir.fieldPrecision)
+            len++;
         } else {
           // Whole string will be accessed.
-          size = internal_strlen((const char *)argp) + 1;
+          len = internal_strlen((const char *)argp) + 1;
         }
-        COMMON_INTERCEPTOR_READ_RANGE(ctx, argp, size);
+        COMMON_INTERCEPTOR_READ_RANGE(ctx, argp, len);
       }
     } else if (size == FSS_WCSLEN) {
       if (void *argp = va_arg(aq, void *)) {
         // FIXME: Properly support wide-character strings (via wcsrtombs).
-        size = 0;
-        COMMON_INTERCEPTOR_READ_RANGE(ctx, argp, size);
+        COMMON_INTERCEPTOR_READ_RANGE(ctx, argp, 0);
       }
     } else {
       // Skip non-pointer args
       SKIP_SCALAR_ARG(&aq, dir.convSpecifier, size);
     }
   }
 }
 
 #endif // SANITIZER_INTERCEPT_PRINTF

compiler-rt/test/sanitizer_common/TestCases/vsnprintf.cpp

+// Test that the common part implementation of *printf interceptors does not
+// cause negative-size-param false positives.
+
+// RUN: %clangxx -O2 %s -o %t
+// RUN: %env_tool_opts=check_printf=1 %run %t 2>&1
+
+// FIXME: The maximum supported allocation size is too platform-specific:
+// REQUIRES: x86_64-target-arch
+
+// FIXME: printf is not intercepted on Windows yet.
+// UNSUPPORTED: target={{.*windows-msvc.*}}
+
+#include <stdarg.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+void write(char *buf, int buf_size, const char *fmt, ...) {
+  va_list args;
+  va_start(args, fmt);
+  vsnprintf(buf, buf_size, fmt, args);
+  va_end(args);
+}
+
+int main() {
+  char buffer[100];
+  const size_t kStrSize = 1UL << 31;
+  char *x = (char *)malloc(kStrSize);
+  memset(x, '=', kStrSize - 1);
+  x[kStrSize - 1] = 0;
+  write(buffer, 100, "%s\n", x);
+  free(x);
+  return 0;
+}