This is an archive of the discontinued LLVM Phabricator instance.

Check for null pointers given to memcpy with ubsan
ClosedPublic

Authored by nlopes on May 11 2015, 12:00 PM.

Details

Summary

gcc 4.9 just got more aggressive and is now exploiting the fact that input pointers to memcpy/memmove cannot be null (even if the size is 0).
This patch adds support to ubsan to check for this UB. Very important since it's already being exploited by gcc.

Diff Detail

Repository
rL LLVM

Event Timeline

nlopes updated this revision to Diff 25492.May 11 2015, 12:00 PM
nlopes retitled this revision from to Check for null pointers in ubsan.
nlopes updated this object.
nlopes edited the test plan for this revision. (Show Details)
nlopes set the repository for this revision to rL LLVM.
nlopes changed the edit policy from "All Users" to "Administrators".
nlopes changed the edit policy from "Administrators" to "All Users".
nlopes retitled this revision from Check for null pointers in ubsan to Check for null pointers given to memcpy with ubsan.
nlopes added a subscriber: Unknown Object (MLST).

Wait a second, won't UBSan handle this automatically if memcpy/memmove are declared with attribute((nonnull)) in the header? Otherwise, is there a change to the standard that imposes these additional constraints on memcpy/memmove?

Wait a second, won't UBSan handle this automatically if memcpy/memmove are
declared with attribute((nonnull)) in the header? Otherwise, is there
a change to the standard that imposes these additional constraints on
memcpy/memmove?

Not really. memcpy/memmove calls are handled by CGBultin and not CGCall.
It's a different code path.
Nuno

Wait a second, won't UBSan handle this automatically if memcpy/memmove are
declared with attribute((nonnull)) in the header? Otherwise, is there
a change to the standard that imposes these additional constraints on
memcpy/memmove?

Not really. memcpy/memmove calls are handled by CGBultin and not CGCall.
It's a different code path.
Nuno

Interesting. I think that if we decide to implement such a check, we shouldn't depend on
attributes specified in the headers, so nonnull-attribute is no longer relevant. There are another
kind of compiler builtins which worth extra checks, and which don't even require headers - e.g. behavior of __builtin_ctz(0)
is undefined. I think we should implement another check kind -fsanitize=builtin that would verify arguments of
various builtin functions.

Wait a second, won't UBSan handle this automatically if memcpy/memmove are
declared with attribute((nonnull)) in the header? Otherwise, is there
a change to the standard that imposes these additional constraints on
memcpy/memmove?

Not really. memcpy/memmove calls are handled by CGBultin and not CGCall.
It's a different code path.
Nuno

Interesting. I think that if we decide to implement such a check, we shouldn't depend on
attributes specified in the headers, so nonnull-attribute is no longer relevant. There are another
kind of compiler builtins which worth extra checks, and which don't even require headers - e.g. behavior of __builtin_ctz(0)
is undefined. I think we should implement another check kind -fsanitize=builtin that would verify arguments of
various builtin functions.

Well, I think in the end they are all in the UB category, so I think they fall in the scope of -fsanitize=undefined.
Nuno

rsmith edited edge metadata.May 24 2015, 9:43 AM

I agree that we should be respecting the __attribute__((nonnull)) on these functions whether or not we emit them as builtin calls; as such, it makes sense to me for this to be under -fsanitize=nonnull-attribute. A couple of minor copy-paste issues and then this looks fine to me, but please wait to make sure that @samsonov is persuaded.

lib/CodeGen/CGBuiltin.cpp
715 ↗(On Diff #25492)

The 0 here should be a 1.

776 ↗(On Diff #25492)

Likewise.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 9 2023, 3:15 PM
Herald added a subscriber: bollu. · View Herald Transcript