[compiler-rt] r284901 - [sanitizers] support strict_string_checks for strncmp

[Kostya Serebryany via llvm-commits]

On Tue, Nov 8, 2016 at 2:30 PM, Nick Lewycky <nlewycky at google.com> wrote:

> On 21 October 2016 at 16:52, Kostya Serebryany via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>> Author: kcc
>> Date: Fri Oct 21 18:52:26 2016
>> New Revision: 284901
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=284901&view=rev
>> Log:
>> [sanitizers] support strict_string_checks for strncmp
>>
>> Added:
>>     compiler-rt/trunk/test/asan/TestCases/strncmp_strict.c
>> Modified:
>>     compiler-rt/trunk/lib/sanitizer_common/sanitizer_common_
>> interceptors.inc
>>
>> Modified: compiler-rt/trunk/lib/sanitizer_common/sanitizer_common_
>> interceptors.inc
>> URL: http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sa
>> nitizer_common/sanitizer_common_interceptors.inc?rev=284901&
>> r1=284900&r2=284901&view=diff
>> ============================================================
>> ==================
>> --- compiler-rt/trunk/lib/sanitizer_common/sanitizer_common_interceptors.inc
>> (original)
>> +++ compiler-rt/trunk/lib/sanitizer_common/sanitizer_common_interceptors.inc
>> Fri Oct 21 18:52:26 2016
>> @@ -304,8 +304,8 @@ INTERCEPTOR(int, strncmp, const char *s1
>>      c2 = (unsigned char)s2[i];
>>      if (c1 != c2 || c1 == '\0') break;
>>    }
>> -  COMMON_INTERCEPTOR_READ_RANGE(ctx, s1, Min(i + 1, size));
>> -  COMMON_INTERCEPTOR_READ_RANGE(ctx, s2, Min(i + 1, size));
>> +  COMMON_INTERCEPTOR_READ_STRING(ctx, s1, Min(i + 1, size));
>> +  COMMON_INTERCEPTOR_READ_STRING(ctx, s2, Min(i + 1, size));
>>
>
> I think this change is wrong, but please double check my analysis. I think
> arguments to strncmp don't need to be NUL terminated like arguments to
> strcmp do.
>
> C99 7.1.1 defines: "A string is a contiguous sequence of characters
> terminated by and including the first null character." Then strcmp is
> defined as a function which "compares the strings" in 7.21.4.2 while
> strncmp is defined to operate over two "possibly null-terminated array"s in
> 7.21.4.4. The bug manifests in COMMON_INTERCEPTOR_READ_STRING calling
> REAL(strlen) on these arguments which aren't guaranteed to be NUL
> terminated.
>
> What do you think?
>

Mmmm. Are these parts equivalent in C99 and C++ standards?
At least here http://www.cplusplus.com/reference/cstring/strncmp/ I read:
str1C string to be compared.str2C string to be compared.

I probably still need to replace this with something like
COMMON_INTERCEPTOR_READ_RANGE((ctx), (s),                       \
      common_flags()->strict_string_checks ? size : (Min(i + 1, size)) )
to avoid calling strlen


>
> Nick
>
>    int result = CharCmpX(c1, c2);
>>    CALL_WEAK_INTERCEPTOR_HOOK(__sanitizer_weak_hook_strncmp,
>> GET_CALLER_PC(), s1,
>>                               s2, size, result);
>>
>> Added: compiler-rt/trunk/test/asan/TestCases/strncmp_strict.c
>> URL: http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/test/
>> asan/TestCases/strncmp_strict.c?rev=284901&view=auto
>> ============================================================
>> ==================
>> --- compiler-rt/trunk/test/asan/TestCases/strncmp_strict.c (added)
>> +++ compiler-rt/trunk/test/asan/TestCases/strncmp_strict.c Fri Oct 21
>> 18:52:26 2016
>> @@ -0,0 +1,26 @@
>> +// Test strict_string_checks option in strncmp function
>> +// RUN: %clang_asan %s -o %t && %run %t 2>&1
>> +// RUN: %env_asan_opts=strict_string_checks=false %run %t 2>&1
>> +// RUN: %env_asan_opts=strict_string_checks=true not %run %t 2>&1 |
>> FileCheck %s
>> +
>> +#include <assert.h>
>> +#include <stdlib.h>
>> +#include <string.h>
>> +
>> +int main(int argc, char **argv) {
>> +  size_t size = 100;
>> +  char fill = 'o';
>> +  char *s1 = (char*)malloc(size);
>> +  memset(s1, fill, size);
>> +  char *s2 = (char*)malloc(size);
>> +  memset(s2, fill, size);
>> +  s1[size - 1] = 'z';
>> +  s2[size - 1] = 'x';
>> +  int r = strncmp(s1, s2, size + 1);
>> +  // CHECK: {{.*ERROR: AddressSanitizer: heap-buffer-overflow on
>> address}}
>> +  // CHECK: READ of size 101
>> +  assert(r == 1);
>> +  free(s1);
>> +  free(s2);
>> +  return 0;
>> +}
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161108/2e78c899/attachment.html>