This is an archive of the discontinued LLVM Phabricator instance.

[ubsan] Add a nullability sanitizer
ClosedPublic

Authored by vsk on Mar 8 2017, 3:18 PM.

Details

Summary

Teach UBSan how to detect violations of the _Nonnull annotation when
passing arguments to callees, in assignments, and in return stmts.

Because _Nonnull does not affect IRGen, the new checks are disabled by
default. The new driver flags are:

-fsanitize=nullability-arg (_Nonnull violation in call)
-fsanitize=nullability-assign (_Nonnull violation in assignment)
-fsanitize=nullability-return (_Nonnull violation in return stmt)
-fsanitize=nullability (all of the above)

This patch builds on top of UBSan's existing support for detecting
violations of the nonnull attributes ('nonnull' and 'returns_nonnull').
I am reusing the compiler-rt support for the existing checks for now.
I have FIXME's for this, and plan on handling them in a follow-up.

One point of note is that the nullability-return check is only allowed
to kick in if all arguments to the function satisfy their nullability
preconditions. This makes it necessary to emit some null checks in the
function body itself.

Testing: check-clang and check-ubsan. I also built some Apple ObjC
frameworks with an asserts-enabled compiler, and verified that we get
valid reports.

Diff Detail

Event Timeline

vsk created this revision.Mar 8 2017, 3:18 PM
aprantl added a subscriber: aprantl.Mar 8 2017, 4:11 PM
aprantl added inline comments.
docs/UndefinedBehaviorSanitizer.rst
98

s/a/an/

99

Can you rephrase this without using 2nd person?

lib/CodeGen/CGCall.cpp
2926

This function could use some comments.

3319

comment what this early return means?

lib/CodeGen/CGDecl.cpp
676

Comment what the check is doing?

1913

Is this a typo, or do you. really want Nullability here? Asking because everywhere else there is a check for ! Nullability && *Nullability == NullabilityKind::NonNull.
Ans should this condition be factored out?

lib/CodeGen/CodeGenFunction.h
1766

The \brief is redundant. We run doxygen with autobrief.

3474

ditto

vsk updated this revision to Diff 91100.Mar 8 2017, 4:14 PM
vsk added reviewers: aprantl, arphaman.
  • Improve the wording of the docs.
  • Drop a weak test from 'ubsan-null-retval.m'.
jroelofs added inline comments.Mar 9 2017, 7:42 AM
lib/CodeGen/CGDecl.cpp
1913

Could just stick if (auto Nullability = in there, and remove the Nullability &&.

lib/CodeGen/CodeGenFunction.cpp
829

Hasn't the if (auto Nullability = already checked the truthiness of the var, making the Nullability && part of the second check redundant?

test/CodeGenObjC/ubsan-null-retval.m
14 ↗(On Diff #91100)

The:

alloca
store
load

part of this looks like it's just making extra work for mem2reg. Is the alloca actually necessary?

vsk marked 8 inline comments as done.Mar 9 2017, 9:27 AM

Thanks for your comments, and sorry for jumping the gun earlier with an updated diff. I'll attach a fixed-up diff shortly.

lib/CodeGen/CGDecl.cpp
1913

'Nullability' should not have been checked for truthiness twice. By factoring out, do you mean moving the check's logic out of EmitParmDecl and into a helper? If so, I'm not sure if that would make the code cleaner, but I'm open to it.

test/CodeGenObjC/ubsan-null-retval.m
14 ↗(On Diff #91100)

Clang likes its allocas :). This is typical -O0 behavior: IIUC it simplifies IRGen. E.g for expressions which take the address of an argument, because there is guaranteed storage for the arg.

vsk updated this revision to Diff 91180.Mar 9 2017, 9:30 AM
  • Add a test for the mixed _Nonnull arg + attribute((nonnull)) arg case.
  • Reword docs per Adrian's comments, fix up doxygen comments, add better code comments, drop redundant "Nullability" truthiness checks.
zaks.anna added inline comments.Mar 9 2017, 10:02 AM
docs/UndefinedBehaviorSanitizer.rst
101

Have you investigated if -fsanitize=nullability-arg is too noisy? I think we should somehow "recommend" return and assignment check to most users. This is not clear from the help and options provided. The main concern here is that someone would try -fsanitize=nullability, see a ton of warnings they don't care about and think that the whole check is too noisy.

I cannot come up with a great solution. Options that come into mind are:

  • Drop "arg" check completely.
  • Remove arg from the nullability group.
  • Have 2 groups for nullability.

We can also wait for concrete data before making a decision here and address this in subsequent commits.

lib/CodeGen/CGDecl.cpp
1909

I would add a comment explaining why this is needed, similar to what you included in the commit message:
"One point of note is that the nullability-return check is only allowed
to kick in if all arguments to the function satisfy their nullability
preconditions. This makes it necessary to emit some null checks in the
function body itself."

Maybe even rename CanCheckRetValNullability into RetValNullabilityPrecondition. I like this more than "CanCheck" because it's just a precondition value. You check if it is satisfied (evaluates to true or false) later when it's used in a branch.

Please make the tests tighter using CHECK-NEXT when possible. Much easier if later anyone needs to debug differences in IR.

docs/UndefinedBehaviorSanitizer.rst
102

This is weird. Please just document each of the nullability-* in isolation. None of the other flags is documenting more than one flag per bullet point, and I think splitting them up here is easy. It also makes it easier to read.
You already added the nullability group below, which mentions it adds the three checks.

141
... and the ``nullability`` group.
test/CodeGenObjC/ubsan-nullability.m
114

CHECK-NEXT?

vsk added a comment.Mar 9 2017, 6:48 PM

One question for Anna (inline). I will update the diff with the documentation/code comments/renaming fixes once I hear back. Thanks again for the comments!

docs/UndefinedBehaviorSanitizer.rst
101

The nullability-arg check was not noisy on a small sample (n=4) of Apple-internal projects. I only collected 11 diagnostics in total, all of which seemed actionable. I.e the issues were not isolated to system headers: the project code actually did violate arg nullability. Based on what I've seen so far, I'd like to start off with including the arg check in a "nullability-full" group. If arg check noisiness becomes an issue, then I'd like to create a new "nullability-partial" group with just the return/assignment checks.

TL;DR: let's rename -fsanitize=nullability to -fsanitize=nullability-full, keep the arg check, and introduce a nullability-partial group later if we need to. Wdyt?

zaks.anna added inline comments.Mar 9 2017, 9:16 PM
docs/UndefinedBehaviorSanitizer.rst
101

Keeping things as is and introducing a new group if this becomes a problem sounds good to me. You could use "nullability" and "nullability-pedantic". That way you do not need to change the name and get to use "nullability".

vsk marked 9 inline comments as done.Mar 10 2017, 11:24 AM

The plan is to start off with -fsanitize=nullability, and then create a nullability-pedantic group later if it's really necessary. I think I've addressed all of the inline comments, and will upload a new diff shortly.

docs/UndefinedBehaviorSanitizer.rst
101

Sgtm.

141

I define the group after the 'undefined' group, so I used a slightly different spelling.

lib/CodeGen/CGDecl.cpp
1909

I added the comment and did the rename. I like 'RetValNullabilityPrecondition'.

test/CodeGenObjC/ubsan-nullability.m
114

I've promoted all the existing 'CHECK' lines to 'CHECK-NEXT's where possible.

vsk updated this revision to Diff 91382.Mar 10 2017, 11:29 AM
vsk marked 4 inline comments as done.
  • Rework documentation, add better code comments, and tighten up some check lines.
zaks.anna accepted this revision.Mar 11 2017, 9:51 AM
This revision is now accepted and ready to land.Mar 11 2017, 9:51 AM
This revision was automatically updated to reflect the committed changes.