On Mon, Oct 16, 2017 at 01:57:59PM +0200, Martin Liška wrote:
> Agree. Do you feel that it's right moment to trigger review process of
> libsanitizer
> changes?
Yes. We don't have that much time left to get it through...
> --- a/libsanitizer/asan/asan_report.cc
> +++ b/libsanitizer/asan/asan_report.cc
> + {
> + uptr shadow_offset1, shadow_offset2;
> + ThreadRegistryLock l(&asanThreadRegistry());
> +
> + // check whether left is a stack memory pointer
> + if (GetStackVariableBeginning(left, &shadow_offset1)) {
> + if (GetStackVariableBeginning(right - 1, &shadow_offset2) &&
> + shadow_offset1 == shadow_offset2)
> + return;
> + else
> + goto do_error;
> + }
Do you need the ThreadRegistryLock for the following calls?
If not, the } should be closed and it should be reindented.
> + // check whether left is a heap memory address
> + HeapAddressDescription hdesc1, hdesc2;
> + if (GetHeapAddressInformation(left, 0, &hdesc1) &&
> + hdesc1.chunk_access.access_type == kAccessTypeInside) {
> + if (GetHeapAddressInformation(right, 0, &hdesc2) &&
> + hdesc2.chunk_access.access_type == kAccessTypeInside &&
> + (hdesc1.chunk_access.chunk_begin
> + == hdesc2.chunk_access.chunk_begin))
> + return;
> + else
> + goto do_error;
> + }
> + // check whether left is an address of a global variable
> + GlobalAddressDescription gdesc1, gdesc2;
> + if (GetGlobalAddressInformation(left, 0, &gdesc1)) {
> + if (GetGlobalAddressInformation(right - 1, 0, &gdesc2) &&
> + gdesc1.PointsInsideASameVariable (gdesc2))
> + return;
> + } else
> + goto do_error;
?? If we don't know anything about the left object, doing a goto do_error;
sounds dangerous, it could be say mmapped region or whatever else.
Though, we could at that spot at least check if right isn't one
of the 3 kinds of regions we track and if yes, error out.
So perhaps move the else goto do_error; inside of the {} and
do
if (GetStackVariableBeginning(right - 1, &shadow_offset2) ||
GetHeapAddressInformation(right, 0, &hdesc2) ||
GetGlobalAddressInformation(right - 1, 0, &gdesc2))
goto do_error;
return;
(if the lock above is released, you'd of course need to retake it for
if (GetStackVariableBeginning(right - 1, &shadow_offset2)).
Jakub
