This is a patch for Issue 346: moar string interceptors: strstr, strcasestr, strcspn, strpbrk.
The initial patch for this issue was: http://reviews.llvm.org/D6056. But it was abandoned and splitted in two parts: New "strict_string_checks" run-time flag and this one.
Details
- Reviewers
kcc glider dvyukov samsonov - Commits
- rG0ca65fd83d50: [sanitizer] More string interceptors: strstr, strcasestr, strspn, strcspn…
rCRT238406: [sanitizer] More string interceptors: strstr, strcasestr, strspn, strcspn…
rL238406: [sanitizer] More string interceptors: strstr, strcasestr, strspn, strcspn…
Diff Detail
- Repository
- rL LLVM
Event Timeline
lib/sanitizer_common/sanitizer_common_interceptors.inc | ||
---|---|---|
269 ↗ | (On Diff #23733) | delete empty line (two consequent empty lines) |
271 ↗ | (On Diff #23733) | delete empty line (that's how it is written everywhere in this file) |
277 ↗ | (On Diff #23733) | Is "r - s1 + len2 + 1" correct? |
279 ↗ | (On Diff #23733) | I am not sure such complex computations are necessary. People should not rely on such things. |
283 ↗ | (On Diff #23733) | add parens around COMMON_INTERCEPTOR_NOTHING_IS_INITIALIZED |
288 ↗ | (On Diff #23733) | drop {} |
294 ↗ | (On Diff #23733) | #define INIT_STRSTR \ COMMON_INTERCEPT_FUNCTION(strstr); \ COMMON_INTERCEPT_FUNCTION(strcasestr); and move below |
306 ↗ | (On Diff #23733) | remove |
310 ↗ | (On Diff #23733) | remove |
312 ↗ | (On Diff #23733) | delete empty line |
315 ↗ | (On Diff #23733) | delete empty line |
322 ↗ | (On Diff #23733) | I don't believe this is necessary. Make it just: |
328 ↗ | (On Diff #23733) | move below (see how it is formatted in other case in this file) |
337 ↗ | (On Diff #23733) | I don't believe this complex code is necessary. Make it just: |
4945 ↗ | (On Diff #23733) | delete |
lib/sanitizer_common/sanitizer_flags.inc | ||
156 ↗ | (On Diff #23733) | do we need all these additional flags? why? |
lib/sanitizer_common/sanitizer_platform_interceptors.h | ||
58 ↗ | (On Diff #23733) | this is unused, delete |
60 ↗ | (On Diff #23733) | delete |
lib/sanitizer_common/sanitizer_common_interceptors.inc | ||
---|---|---|
279 ↗ | (On Diff #23733) | Do you mean the following: (ctx, s2, len2, r ? len2 + 1 : (len2 <= len1 ? 1 : len1 + 1)); ? |
322 ↗ | (On Diff #23733) | Why not? |
337 ↗ | (On Diff #23733) | The same - please explain. My intention was to avoid strict checks when possible. |
lib/sanitizer_common/sanitizer_flags.inc | ||
156 ↗ | (On Diff #23733) | It was discussed in initial patch for this http://reviews.llvm.org/D6056#92330: glider suggested to have individual flags. I agree, it's more flexible. |
lib/sanitizer_common/sanitizer_platform_interceptors.h | ||
58 ↗ | (On Diff #23733) | It's my bug, SANITIZER_INTERCEPT_STRCASESTR must be used in sanitizer_common_interceptors.inc cause indeed there is no strcasestr on Windows. I'll fix it. The rest is always 1 so I agree it could be removed. I wonder why there are other defines allways equal to 1 in sanitizer_platform_interceptors.h? |
lib/sanitizer_common/sanitizer_common_interceptors.inc | ||
---|---|---|
279 ↗ | (On Diff #23733) | The worst case is strlen(s2)+1. An implementation is allowed to read that much. |
322 ↗ | (On Diff #23733) | Because these are very complex expressions with lots of corner cases and potential off-by-ones. |
lib/sanitizer_common/sanitizer_flags.inc | ||
156 ↗ | (On Diff #23733) | Okay |
lib/sanitizer_common/sanitizer_platform_interceptors.h | ||
58 ↗ | (On Diff #23733) | I mean delete them because they are unused, not because they are always 1. |
lib/sanitizer_common/sanitizer_common_interceptors.inc | ||
---|---|---|
279 ↗ | (On Diff #23733) |
I understand the non-strict mode as ability to check less then full string length when it's possible. It shouldn't rely on best case, it should check at least as much as needed for getting the result. I think it's better to check more bytes than to miss incorrect read. If proposed read check is inaccurate I agree it's better to replace with full length check. |
322 ↗ | (On Diff #23733) |
The "strict_string_checks" flag was introduced exactly cause user may expect such behaviour despite of documentation. Which are corner cases here? Can I just cover them? |
lib/sanitizer_common/sanitizer_common_interceptors.inc | ||
---|---|---|
322 ↗ | (On Diff #23733) | I guess Dmitry means that there is a tradeoff between functionality and code complexity. And he'd prefer to drop overly complex checks in favor of simplicity. |
lib/sanitizer_common/sanitizer_common_interceptors.inc | ||
---|---|---|
322 ↗ | (On Diff #23733) | Yes, what Yury said. "r ? internal_strchr(s2, *r) - s2 + 1 : *s1 ? REAL(strlen)(s2) + 1 : 0" is complex without providing any obvious benefit. If anything, non-strict mode check at _most_ (rather than at least) as much as needed for getting the result. But what is required to get the result? It depends on the implementation of the function in libc. |
Hi,
I've uploaded new patch with fixes suggested by Dmitry. Could you please take a look? Thank you.
Thank you. I've uploaded new patch with strspn interceptors added. I moved both strspn and strcspn under the same flag intercept_strspn and one macro SANITIZER_INTERCEPT_STRSPN.
I'll submit tomorrow (if no objections from Dima) so that we are around to cope with bugs.