[PATCH] D31457: [asan] Add strndup/__strndup interceptors if targeting linux.

[pierre gousseau via Phabricator via llvm-commits]

pgousseau added a comment.

Thanks Filipe/Eugenis for the comments, one question I have is, since my change modifies the behaviour of msan strndup interceptor (cf. added msan/strndup.cc test), should I keep the msan change as part of this patch or do you prefer a separate review?



================
Comment at: lib/msan/msan_interceptors.cc:1339
+    GET_STORE_STACK_TRACE;                              \
+    char *res = REAL(strndup)(src, size);               \
+    CopyShadowAndOrigin(res, src, size, &stack);        \
----------------
eugenis wrote:
> I don't like that the code tries to combine two very different implementations in one "common" interceptor - one that calls REAL(strlen) and one that uses malloc directly.
> 
> Common interceptor should implement the logic of strndup, and call individual sanitizers only to update their specific state - READ_RANGE at the start, and something like COPY_RANGE after.
> 
> I think the common implementation should use malloc directly. 
> 
Makes sense. Will update patch.


================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors.inc:315
+  COMMON_INTERCEPTOR_ENTER(ctx, strndup, s, size);
+  uptr from_length = REAL(strlen)(s);
+  uptr copy_length = Min(size, from_length + 1);
----------------
eugenis wrote:
> this should use strnlen, like the msan interceptor it is replacing
Makes sense thanks.


================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors.inc:330
+  void *ctx;
+  COMMON_INTERCEPTOR_ENTER(ctx, strndup, s, size);
+  uptr from_length = REAL(strlen)(s);
----------------
filcab wrote:
> Should the context be `__strndup`?
> It's probably ok as it is if the `__strndup` function is not supposed to be user-visible.
Yes I think strndup is preferred because this is also the case with strdup/strndup in asan_interceptors.cc


================
Comment at: lib/sanitizer_common/tests/sanitizer_test_utils.h:127
 
+#if defined(__linux__) && !defined(__ANDROID__)
+# define SANITIZER_TEST_HAS_STRNDUP 1
----------------
eugenis wrote:
> filcab wrote:
> > We should also enter here if `defined(__APPLE__)` and if `defined(__FreeBSD__)`, I think.
> > Can't think of more OS we support that should be here.
> > 
> > I'm not sure we should be taking Android out of the picture, as it seems `strndup` is available, AFAICT. @kcc/@vitalybuka (or others who might be working on Android) can you confirm if we should expect `strndup` to be available?
> Yes, strndup is available on Android.
Sounds good will do.


================
Comment at: test/asan/TestCases/strndup_oob_test.cc:9
+
+// REQUIRES: linux
+// XFAIL: android
----------------
filcab wrote:
> I don't think this should require Linux. It should have Windows as unsupported, but should work on all (?) POSIX.
> Maybe just put it in TestCases/Posix?
Makes sense thanks!


https://reviews.llvm.org/D31457