patch: make ubsan report error summaries
Nick Lewycky
nlewycky at google.com
Wed Oct 23 13:02:55 PDT 2013
On 23 October 2013 11:21, Richard Smith <richard at metafoo.co.uk> wrote:
> On Wed, Oct 23, 2013 at 9:14 AM, Alexey Samsonov <samsonov at google.com>wrote:
>
>>
>> On Wed, Oct 23, 2013 at 2:21 PM, Nick Lewycky <nlewycky at google.com>wrote:
>>
>>> On 23 October 2013 03:05, Alexey Samsonov <samsonov at google.com> wrote:
>>>
>>>> Hi!
>>>>
>>>> + if (Level == DL_Error) {
>>>> + InternalScopedBuffer<char> summary(1024);
>>>> + internal_snprintf(summary.data(), summary.size(),
>>>> + "UndefinedBehaviorSanitizer: %s", Flag);
>>>> + switch (Loc.getKind()) {
>>>> + case Location::LK_Null:
>>>> + break;
>>>> + case Location::LK_Source: {
>>>> + SourceLocation SLoc = Loc.getSourceLocation();
>>>> + internal_snprintf(summary.data(), summary.size(), "%s
>>>> %s:%u:%u",
>>>> + summary.data(),
>>>> + SLoc.getFilename(), SLoc.getLine(),
>>>> SLoc.getColumn());
>>>> + break;
>>>> + }
>>>> + case Location::LK_Module: {
>>>> + ModuleLocation MLoc = Loc.getModuleLocation();
>>>> + internal_snprintf(
>>>> + summary.data(), summary.size(), "%s (%s+0x%zx)",
>>>> summary.data(),
>>>> + StripPathPrefix(MLoc.getModuleName(),
>>>> +
>>>> __sanitizer::common_flags()->strip_path_prefix),
>>>> + MLoc.getOffset());
>>>> + break;
>>>> + }
>>>> + case Location::LK_Memory:
>>>> + internal_snprintf(summary.data(), summary.size(),
>>>> + " %p", summary.data(),
>>>> Loc.getMemoryLocation());
>>>> + break;
>>>> + }
>>>> + __sanitizer_report_error_summary(summary.data());
>>>> + }
>>>>
>>>> This code looks too similar to renderLocation() in ubsan_diag.cc.
>>>> Probably they both
>>>> should use PrintSourceLocation() and PrintModuleAndOffset() from
>>>> sanitizer_common.cc, and the
>>>> latter two functions should be generalized to work with a buffer
>>>> instead of simply printing the error message.
>>>>
>>>
>>> It's not just renderLocation, the code in this if-statement is a second
>>> copy of the function up to before the renderMemorySnippet call.
>>>
>>> Richard doesn't want this code to use any buffer at all (and indeed, up
>>> until this patch ubsan doesn't). Is there any way we can preserve that?
>>> Making PrintSourceLocation and PrintModuleAndOffset use intermediate
>>> buffers is going in the wrong direction -- it'd be best if we could avoid
>>> needing a buffer, because when this code runs the program is failing and we
>>> might not be able to get a buffer.
>>>
>>
>> It's a pretty strict limitation, this makes error reporting a lot harder.
>> Usually we think that we can allocate (or at least mmap) a buffer. In fact,
>> if we're using an internal symbolizer, it does all sorts of complex system
>> calls, memory (de)allocation etc, which happen during the report printing.
>> By the way, I've investigated a number of UBSan failures today and think
>> that we should really move toward better error reports in UBSan - at least
>> we should print the stack traces when error happens (if
>> -fno-sanitize-recover is provided).
>>
>
> Definitely. The thing blocking this is that sanitizer_common's backtrace
> functionality doesn't work for a ubsan-instrumented binary, because we
> don't have any symbol interposition for ubsan (we have avoided this,
> because we didn't want to require instrumentation of the whole program) and
> the backtrace functionality currently requires us to instrument process and
> thread startup.
>
There are already places where UBSan could benefit from access to the asan
shadow map, to avoid wild reads when in the error handlers. Could we make
ubsan detect then use the instrumentation when it's being provided by
another sanitizer?
Nick
>
>>> Could we break up __sanitizer_report_error_summary into two functions,
>>> one that takes text as a stream, and a second one which flushes operation?
>>> I think the only problem is that it wouldn't be thread safe.
>>>
>>
>> I think CommonSanitizerReportMutex should protect us here - we use it to
>> make sure two reports (from different or same sanitizer) are not intermixed.
>>
>>
>>>
>>> Nick
>>>
>>> On Wed, Oct 23, 2013 at 1:21 PM, Nick Lewycky <nlewycky at google.com>wrote:
>>>>
>>>>> On 22 October 2013 22:07, Nick Lewycky <nlewycky at google.com> wrote:
>>>>>
>>>>>> On 22 October 2013 21:18, Nick Lewycky <nlewycky at google.com> wrote:
>>>>>>
>>>>>>> The attached patch makes ubsan emit summaries of errors it
>>>>>>> encounters. The format of these summaries is:
>>>>>>> UndefinedBehaviourSanitizer: signed-integer-overflow file:49:7
>>>>>>> where the string is the flag name. Most of the patch is adding the
>>>>>>> flag names to all the reports all over.
>>>>>>>
>>>>>>
>>>>>> I've noticed a small bug, for load-invalid-value we always pick
>>>>>> "enum" and never "bool". I would guess that's because
>>>>>> ASTContext::getTypeSize(BoolTy) returns 8 instead of 1?
>>>>>>
>>>>>> Richard, thoughts?
>>>>>>
>>>>>
>>>>> Updated patch attached. It now detects bool sanitizer by looking at
>>>>> the Type as a string, and is otherwise updated for the changes in
>>>>> sanitizer-common.
>>>>>
>>>>> Nick
>>>>>
>>>>>
>>>>>>
>>>>>> Nick
>>>>>>
>>>>>> This patch is stacked on top of
>>>>>>> http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20131021/091535.html ,
>>>>>>> or else ubsan's tests will fail.
>>>>>>>
>>>>>>> Please review!
>>>>>>>
>>>>>>> Nick
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> cfe-commits mailing list
>>>>> cfe-commits at cs.uiuc.edu
>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>>>
>>>>>
>>>>
>>>>
>>>> --
>>>> Alexey Samsonov, MSK
>>>>
>>>
>>>
>>
>>
>> --
>> Alexey Samsonov, MSK
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131023/e53fb3d8/attachment.html>
More information about the cfe-commits
mailing list