llvm dev - May 2018 - RFC: Implementing -fno-delete-null-pointer-checks in clang

On 5/12/2018 9:23 PM, Philip Reames via llvm-dev wrote:
> Fair warning, the following is a devil's advocate position, but
it's
> also a serious question.
>
> Given the entire point of this flag appears to be bug mitigation, why 
> not frame this as a sanitizer?  If we had a hypothetical 
> -fsanitize=dereference which tried to catch dereferenced pointers 
> derived from null, wouldn't that handle the case at hand?
It's called "-fsanitize=null": it catches stuff like
"x[3]" where x is
null. It's not quite complete; we don't check for arithmetic on a null 
pointer.

Yes, that would handle the situation in question, but putting implicit 
null checks all over the place is pretty expensive; I don't think most 
people would turn that on in production.

-Eli

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux
Foundation Collaborative Project

On Mon, May 14, 2018 at 12:07 PM Friedman, Eli <efriedma at
codeaurora.org>
wrote:
> On 5/12/2018 9:23 PM, Philip Reames via llvm-dev wrote:
> > Fair warning, the following is a devil's advocate position, but
it's
> > also a serious question.
> >
> > Given the entire point of this flag appears to be bug mitigation, why
> > not frame this as a sanitizer?  If we had a hypothetical
> > -fsanitize=dereference which tried to catch dereferenced pointers
> > derived from null, wouldn't that handle the case at hand?
>
> It's called "-fsanitize=null": it catches stuff like
"x[3]" where x is
> null. It's not quite complete; we don't check for arithmetic on a
null
> pointer.
>
> Yes, that would handle the situation in question, but putting implicit
> null checks all over the place is pretty expensive; I don't think most
> people would turn that on in production.
>
> -Eli
>
We had a similar discussion on an internal thread a while back if we can
use "-fsanitize=null" where clang
would generate ud2 instruction for null pointer dereferences.
Unfortunately, this doesn't work in kernel context.

Quoting the reply from our kernel team:

"It will not cause a kernel panic: it's an exception trigger, and
it's
up to the exception handler to decide if it will return (WARN) or not
(BUG). In the referenced function, this is calling WARN_ON() which
will resume execution. (And note that the BUG() implementations are
specifically marked with __attribute__((noreturn)). "
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180514/5d8137d6/attachment.html>

On 05/14/2018 12:07 PM, Friedman, Eli wrote:
> On 5/12/2018 9:23 PM, Philip Reames via llvm-dev wrote:
>> Fair warning, the following is a devil's advocate position, but
it's
>> also a serious question.
>>
>> Given the entire point of this flag appears to be bug mitigation, why 
>> not frame this as a sanitizer?  If we had a hypothetical 
>> -fsanitize=dereference which tried to catch dereferenced pointers 
>> derived from null, wouldn't that handle the case at hand?
>
> It's called "-fsanitize=null": it catches stuff like
"x[3]" where x is
> null. It's not quite complete; we don't check for arithmetic on a
null
> pointer.
>
> Yes, that would handle the situation in question, but putting implicit 
> null checks all over the place is pretty expensive; I don't think most 
> people would turn that on in production.
(again, knowingly playing devil's advocate)

But wouldn't it catch - during development, before release - the bug 
which motivates the desire for the mitigation?  If so, why worry about 
the mitigation?  Is the concern insufficient testing/coverage?

And alternatively, the performance overhead could be likely to be 
improved if someone wanted to spend some time doing so.   Between fully 
exploiting implicit null checks (support in tree today) and imprecise 
fault locations(*), I suspect the overhead could be made quite small.

* Terminology: "implicit null check" is simply constructing a load or 
store which is guaranteed to fault if the preceding null check would 
have failed.  "imprecise fault location" is allowing the failure to 
happen earlier or later in the execution so long as additional side 
effect is not visible.  If you're okay with earlier side effects not 
being visible, that opens up a lot of flexibility. Similar, allowing 
some side effects  (but not say, IO) does as well.

Philip

On 5/14/2018 12:11 PM, Manoj Gupta wrote:
>
>
> On Mon, May 14, 2018 at 12:07 PM Friedman, Eli 
> <efriedma at codeaurora.org <mailto:efriedma at
codeaurora.org>> wrote:
>
>     On 5/12/2018 9:23 PM, Philip Reames via llvm-dev wrote:
>     > Fair warning, the following is a devil's advocate position,
but
>     it's
>     > also a serious question.
>     >
>     > Given the entire point of this flag appears to be bug
>     mitigation, why
>     > not frame this as a sanitizer?  If we had a hypothetical
>     > -fsanitize=dereference which tried to catch dereferenced pointers
>     > derived from null, wouldn't that handle the case at hand?
>
>     It's called "-fsanitize=null": it catches stuff like
"x[3]" where
>     x is
>     null. It's not quite complete; we don't check for arithmetic on
a
>     null
>     pointer.
>
>     Yes, that would handle the situation in question, but putting
>     implicit
>     null checks all over the place is pretty expensive; I don't think
>     most
>     people would turn that on in production.
>
>     -Eli
>
>
> We had a similar discussion on an internal thread a while back if we 
> can use "-fsanitize=null" where clang
> would generate ud2 instruction for null pointer dereferences. 
> Unfortunately, this doesn't work in kernel context.
>
> Quoting the reply from our kernel team:
>
> "It will not cause a kernel panic: it's an exception trigger, and
it's
> up to the exception handler to decide if it will return (WARN) or not
> (BUG). In the referenced function, this is calling WARN_ON() which
> will resume execution. (And note that the BUG() implementations are
> specifically marked with __attribute__((noreturn)). "
If the kernel can't use -fsanitize-trap, it could use some alternative 
like "-fsanitize=null -fno-sanitize-recover=null 
-fsanitize-minimal-runtime".  That doesn't seem like a fundamental flaw
in the approach.

-Eli

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux
Foundation Collaborative Project

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180514/a7a1e1bf/attachment.html>