On Fri, Oct 13, 2017 at 01:01:37PM +0200, Martin Liška wrote:
> @@ -3826,6 +3827,18 @@ pointer_diff (location_t loc, tree op0, tree op1)
> pedwarn (loc, OPT_Wpointer_arith,
> "pointer to a function used in subtraction");
>
> + if (sanitize_flags_p (SANITIZE_POINTER_SUBTRACT))
> + {
> + gcc_assert (current_function_decl != NULL_TREE);
> +
> + op0 = c_fully_fold (op0, false, NULL);
> + op1 = c_fully_fold (op1, false, NULL);
> +
> + tree tt = builtin_decl_explicit (BUILT_IN_ASAN_POINTER_SUBTRACT);
> + *instrument_expr
> + = build_call_expr_loc (loc, tt, 2, op0, op1);
> + }
If op0 or op1 have some embedded side-effects, won't you evaluate them
multiple times? I'd expect you need to c_save_expr them and make sure the
actual pointer difference expression uses the result of the save expr too.
> --- a/libsanitizer/asan/asan_report.cc
> +++ b/libsanitizer/asan/asan_report.cc
> @@ -344,14 +344,68 @@ static INLINE void CheckForInvalidPointerPair(void *p1,
> void *p2) {
> if (!flags()->detect_invalid_pointer_pairs) return;
> uptr a1 = reinterpret_cast<uptr>(p1);
> uptr a2 = reinterpret_cast<uptr>(p2);
> - AsanChunkView chunk1 = FindHeapChunkByAddress(a1);
> - AsanChunkView chunk2 = FindHeapChunkByAddress(a2);
> - bool valid1 = chunk1.IsAllocated();
> - bool valid2 = chunk2.IsAllocated();
> - if (!valid1 || !valid2 || !chunk1.Eq(chunk2)) {
> - GET_CALLER_PC_BP_SP;
> - return ReportInvalidPointerPair(pc, bp, sp, a1, a2);
> +
> + if (a1 == a2)
> + return;
> +
> + uptr offset = a1 < a2 ? a2 - a1 : a1 - a2;
> + uptr left = a1 < a2 ? a1 : a2;
> + uptr right = a1 < a2 ? a2 : a1;
> + if (offset <= 2048) {
> + if (__asan_region_is_poisoned (left, offset) == 0)
> + return;
> + }
If offset <= 2048 and something is poisoned in between, then it is
clearly a failure, so you should either goto the error or duplicate
the two lines inside of the outer if above.
> +
> + uptr shadow_offset1, shadow_offset2;
> + bool valid1, valid2;
> + {
> + ThreadRegistryLock l(&asanThreadRegistry());
> + valid1 = GetStackVariableBeginning(left, &shadow_offset1);
> }
> +
> + // check whether a1 is a stack memory pointer
> + if (valid1) {
> + {
> + ThreadRegistryLock l(&asanThreadRegistry());
> + valid2 = GetStackVariableBeginning(right - 1, &shadow_offset2);
Why do you take the registery lock twice? Just do:
{
ThreadRegistryLock l(&asanThreadRegistry());
if (GetStackVariableBeginning(left, &shadow_offset1))
{
if (GetStackVariableBeginning(right - 1, &shadow_offset2) &&
shadow_offset1 == shadow_offset2)
return;
// goto do_error; or:
GET_CALLER_PC_BP_SP;
ReportInvalidPointerPair(pc, bp, sp, a1, a2);
return;
}
}
> + }
> +
> + if (valid2 && shadow_offset1 == shadow_offset2)
> + return;
> + }
> + // check whether a1 is a heap memory address
> + else {
> + HeapAddressDescription hdesc1;
> + valid1 = GetHeapAddressInformation(a1, 0, &hdesc1);
> +
> + if (valid1 && hdesc1.chunk_access.access_type == kAccessTypeInside) {
> + HeapAddressDescription hdesc2;
> + valid2 = GetHeapAddressInformation(a2, 0, &hdesc2);
Again, no need for valid1/valid2. Why do you use above left and right - 1
and here a1 and a2? Shouldn't it be always left and right - 1?
Jakub
