This is an archive of the discontinued LLVM Phabricator instance.

[ubsan] Improve diagnostics for return value checks (clang)
ClosedPublic

Authored by vsk on Jun 16 2017, 3:21 PM.

Details

Summary

This patch makes ubsan's nonnull return value diagnostics more precise,
which makes the diagnostics more useful when there are multiple return
statements in a function. Example:

1 |__attribute__((returns_nonnull)) char *foo() {
2 |  if (...) {
3 |    return expr_which_might_evaluate_to_null();
4 |  } else {
5 |    return another_expr_which_might_evaluate_to_null();
6 |  }
7 |} // <- The current diagnostic always points here!

runtime error: Null returned from Line 7, Column 2!

With this patch, the diagnostic would point to either Line 3, Column 5
or Line 5, Column 5.

This is done by emitting source location metadata for each return
statement in a sanitized function. The runtime is passed a pointer to
the appropriate metadata so that it can prepare and deduplicate reports.

Compiler-rt patch (with more tests): https://reviews.llvm.org/D34298

Diff Detail

Event Timeline

vsk created this revision.Jun 16 2017, 3:21 PM
arphaman edited edge metadata.Jun 22 2017, 3:33 AM

It looks like if we have a function without the return (like the sample below), we will pass in a 0 as the location pointer. This will prevent a report of a runtime error as your compiler-rt change ignores the location pointers that are nil. Is this a bug or is this the intended behaviour?

int *_Nonnull nonnull_retval1(int *p) {
}
filcab edited edge metadata.Jun 22 2017, 5:35 AM

Splitting the attrloc from the useloc might make sense since we would be able to emit attrloc just once. But I don't see why we need to store/load those pointers in runtime instead of just caching the Constant* in CodeGenFunction.
I'd also like to have some asserts and explicit resets to nullptr after use on the ReturnLocation variable, if possible.

lib/CodeGen/CGStmt.cpp
1035

Can't you just keep the Constant* around and use it later for the static data? Instead of creating a global var and have runtime store/load?

lib/CodeGen/CodeGenFunction.h
1412

Maybe CurrentReturnLocation?

vsk added a comment.Jun 22 2017, 10:39 AM

It looks like if we have a function without the return (like the sample below), we will pass in a 0 as the location pointer. This will prevent a report of a runtime error as your compiler-rt change ignores the location pointers that are nil. Is this a bug or is this the intended behaviour?

int *_Nonnull nonnull_retval1(int *p) {
}

This is the intended behavior (I'll add a test). Users should not see a "null return value" diagnostic here. There is another check, -fsanitize=return, which can catch this issue.

@filcab --

Splitting the attrloc from the useloc might make sense since we would be able to emit attrloc just once. But I don't see why we need to store/load those pointers in runtime instead of just caching the Constant* in CodeGenFunction.

The source locations aren't constants. The ubsan runtime uses a bit inside of source location structures as a flag. When an issue is diagnosed at a particular source location, that bit is atomically set. This is how ubsan implements issue deduplication.

I'd also like to have some asserts and explicit resets to nullptr after use on the ReturnLocation variable, if possible.

Resetting Address fields in CodeGenFunction doesn't appear to be an established practice. Could you explain what this would be in aid of?

In D34299#788152, @vsk wrote:

It looks like if we have a function without the return (like the sample below), we will pass in a 0 as the location pointer. This will prevent a report of a runtime error as your compiler-rt change ignores the location pointers that are nil. Is this a bug or is this the intended behaviour?

int *_Nonnull nonnull_retval1(int *p) {
}

This is the intended behavior (I'll add a test). Users should not see a "null return value" diagnostic here. There is another check, -fsanitize=return, which can catch this issue.

Ok. However, when the source is not using C++, the check for -fsanitize=return won't be emitted. So we will end up with a call to __ubsan_handle_nullability_return_abort without a location, which won't report a diagnostic, but the program will crash because compiler-rt will call abort. This seems like a regression, since previously in C mode this diagnostic was reported.

@filcab --

Splitting the attrloc from the useloc might make sense since we would be able to emit attrloc just once. But I don't see why we need to store/load those pointers in runtime instead of just caching the Constant* in CodeGenFunction.

The source locations aren't constants. The ubsan runtime uses a bit inside of source location structures as a flag. When an issue is diagnosed at a particular source location, that bit is atomically set. This is how ubsan implements issue deduplication.

I'd also like to have some asserts and explicit resets to nullptr after use on the ReturnLocation variable, if possible.

Resetting Address fields in CodeGenFunction doesn't appear to be an established practice. Could you explain what this would be in aid of?

In D34299#788152, @vsk wrote:

The source locations aren't constants. The ubsan runtime uses a bit inside of source location structures as a flag. When an issue is diagnosed at a particular source location, that bit is atomically set. This is how ubsan implements issue deduplication.

It's still an llvm::Constant. Just like in StaticData, in line 2966.
Basically, I don't see why we need to add the store/load and an additional indirection, since the pointer is constant, and we can just emit the static data as before.
We're already doing Data->Loc.acquire(); for the current version (and all the other checks).

I'd also like to have some asserts and explicit resets to nullptr after use on the ReturnLocation variable, if possible.

Resetting Address fields in CodeGenFunction doesn't appear to be an established practice. Could you explain what this would be in aid of?

It would be a sanity check and help with code reading/keeping in mind the lifetime of the information. I'm ok with that happening only on !NDEBUG builds.

Reading the code, I don't know if a ReturnLocation might end up being used for more than one checks. If it's supposed to be a different one per check, etc.
If it's only supposed to be used once, I'd rather set it to nullptr right after use (at least when !NDEBUG), and assert(!ReturnLocation) before setting. It's not a big deal, but would help with making sense of the flow of information when debugging.

vsk added a comment.Jun 22 2017, 1:55 PM
In D34299#788152, @vsk wrote:

The source locations aren't constants. The ubsan runtime uses a bit inside of source location structures as a flag. When an issue is diagnosed at a particular source location, that bit is atomically set. This is how ubsan implements issue deduplication.

It's still an llvm::Constant. Just like in StaticData, in line 2966.
Basically, I don't see why we need to add the store/load and an additional indirection, since the pointer is constant, and we can just emit the static data as before.

My earlier response made the assumption that you wanted a copy of the source location passed to the runtime handler, by-value. I see now that you're wondering why the source locations aren't part of the static data structure. That's because the location of the return statement isn't known at compile time, i.e it's not static data. The location depends on which return statement is executed.

We're already doing Data->Loc.acquire(); for the current version (and all the other checks).

The other checks do not allow the source location within a static data object to change.

I'd also like to have some asserts and explicit resets to nullptr after use on the ReturnLocation variable, if possible.

Resetting Address fields in CodeGenFunction doesn't appear to be an established practice. Could you explain what this would be in aid of?

It would be a sanity check and help with code reading/keeping in mind the lifetime of the information. I'm ok with that happening only on !NDEBUG builds.

Reading the code, I don't know if a ReturnLocation might end up being used for more than one checks. If it's supposed to be a different one per check, etc.
If it's only supposed to be used once, I'd rather set it to nullptr right after use (at least when !NDEBUG), and assert(!ReturnLocation) before setting. It's not a big deal, but would help with making sense of the flow of information when debugging.

Sure, I'll reset it to Address::invalid().

vsk updated this revision to Diff 103632.Jun 22 2017, 2:23 PM
vsk marked an inline comment as done.

Handle functions without return statements correctly (fixing an issue pointed out by @arphaman). Now, the instrumentation always checks that we have a valid return location before calling the runtime. I added tests for this on the clang side: we can't test it on the compiler-rt side, because functions without return statements can cause stack corruption / crashes on Darwin.

lib/CodeGen/CGStmt.cpp
1035

I hope I've cleared this up, but: we need to store the source location constant _somewhere_, before we emit the return value check. That's because we can't infer which return location to use at compile time.

lib/CodeGen/CodeGenFunction.h
1412

I'd prefer to keep it the way it is, for consistency with the "ReturnValue" field.

Ok, so now the null check return.sloc.load won't call the checker in compiler-rt and so the program won't abort and won't hit the unreachable. I have one question tough:

This patch changes the behavior of this sanitizer for the example that I gave above. Previously a runtime diagnostic was emitted, but now there is none. While I'm not saying that the previous behaviour was correct, I'm wondering if the new behaviour is right. I think that for C++ it makes sense, but I don't know the right answer for C. I'm leaning more towards the new behaviour, since technically in C falling off without returning a value is not UB unless that return value is used by the caller. But at the same time, since we don't diagnose return UB for C, maybe it's still worth diagnosing this particular issue? The user might not catch it otherwise at all (or they might catch it later when they try to access it, but by that point they might not know where the pointer came from). WDYT?

vsk added a comment.Jun 23 2017, 9:01 AM

Ok, so now the null check return.sloc.load won't call the checker in compiler-rt and so the program won't abort and won't hit the unreachable. I have one question tough:

This patch changes the behavior of this sanitizer for the example that I gave above.

Yes, in the case where there is no explicit return, and the return value happens to be null.

Previously a runtime diagnostic was emitted, but now there is none. While I'm not saying that the previous behaviour was correct, I'm wondering if the new behaviour is right. I think that for C++ it makes sense, but I don't know the right answer for C. I'm leaning more towards the new behaviour, since technically in C falling off without returning a value is not UB unless that return value is used by the caller. But at the same time, since we don't diagnose return UB for C, maybe it's still worth diagnosing this particular issue? The user might not catch it otherwise at all (or they might catch it later when they try to access it, but by that point they might not know where the pointer came from). WDYT?

Without seeing what the caller does with the result, the return value check can't do a good job of diagnosing missing returns. I think clang should emit a diagnostic in the example above, and -Wreturn-type does. So I think the new behavior is appropriate.

Fair enough. LGTM from my side.

This revision was automatically updated to reflect the committed changes.
In D34299#788427, @vsk wrote:

I hope I've cleared this up, but: we need to store the source location constant _somewhere_, before we emit the return value check. That's because we can't infer which return location to use at compile time.

Yes, sorry about that. I was thinking about the code the wrong way around. I'm ok with this patch and how it got in. Sorry for the delay in replying.