[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

lebedev.ri added a comment. In D67122#1703616 , @lebedev.ri wrote: > http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap-ubsan/builds/15329/steps/annotate/logs/stdio > appears to contain "only" 3 distinct issues. > I've pushed those 3 fixe

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

lebedev.ri added a comment. http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap-ubsan/builds/15329/steps/annotate/logs/stdio appears to contain "only" 3 distinct issues. I've pushed those 3 fixes, but maybe that is not enough, we'll see. Repository: rG LLVM Github Monorepo CHA

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

lebedev.ri added a comment. Not unexpectedly, this broke sanitizer-x86_64-linux-bootstrap-ubsan. I'm going to attempt to address uncovered issues Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67122/new/ https://reviews.llvm.org/D67122 __

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

lebedev.ri added a comment. In D67122#1703482 , @russell.gallop wrote: > This is failing the sanitizer lint check: > > http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux/builds/23819/steps/64-bit%20check-sanitizer/logs/stdio > > /compiler-rt/lib/u

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

russell.gallop added a comment. This is failing the sanitizer lint check: http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux/builds/23819/steps/64-bit%20check-sanitizer/logs/stdio /compiler-rt/lib/ubsan/ubsan_checks.inc:22: Lines should be <= 80 characters long [whitespace/line_length] [

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

rupprecht added a comment. Just wanna say huge +1 for this patch. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67122/new/ https://reviews.llvm.org/D67122 ___ cfe-commits mailing list cfe-commi

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

rsmith accepted this revision. rsmith added inline comments. This revision is now accepted and ready to land. Comment at: clang/docs/ReleaseNotes.rst:66 + defined if it adds a non-zero offset (or in C, any offset) to a null pointer, + or that forms a null pointer by subtracting

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

lebedev.ri added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:4657 + Builder.GetInsertBlock()->getParent(), PtrTy->getPointerAddressSpace()); + // Check for overflows unless the GEP got constant-folded, + // and only in the default address space

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

rsmith added a comment. Looks fine to me with some doc improvements. Comment at: clang/docs/ReleaseNotes.rst:64-66 + non-zero offset to ``nullptr`` (or making non-``nullptr`` a ``nullptr``, + by subtracting pointer's integral value from the pointer itself; in C, also, + appl

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

lebedev.ri added a comment. Ping. Friendly remainder that the unsanitized UB is still being miscompiled. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67122/new/ https://reviews.llvm.org/D67122 ___ cfe

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

xbolva00 added a comment. &a[0] is really UB in C? Oh =[ Can we have clang warning also for this case? And suggest “a”? Very common code, I think. Hopefully llvm’s optimizers do not exploit this case. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

lebedev.ri added a comment. In D67122#1691307 , @lebedev.ri wrote: > Ping. > After talking with @aaron.ballman, revised the check to also handle the C > semantics ("ptr+0 is UB"). And that resulted in one more occurrence in test-suite - rL373486

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

lebedev.ri updated this revision to Diff 45. lebedev.ri marked 6 inline comments as done. lebedev.ri added a comment. Patch updated - mainly wording changes only. It isn't clear that for C the rule is actually different from C++, if it is it does not make much sense and is irrelevant for LLVM

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

rsmith added inline comments. Comment at: clang/docs/ReleaseNotes.rst:63 +* As per C++ and C Standards (C++: ``[expr.add]``; C17: 6.5.6p8), applying + non-zero offset to ``nullptr`` (or making non-``nullptr`` a ``nullptr``, aaron.ballman wrote: > rsmith wrote:

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

aaron.ballman added inline comments. Comment at: clang/docs/ReleaseNotes.rst:63 +* As per C++ and C Standards (C++: ``[expr.add]``; C17: 6.5.6p8), applying + non-zero offset to ``nullptr`` (or making non-``nullptr`` a ``nullptr``, rsmith wrote: > In C, even ad

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

lebedev.ri marked 2 inline comments as done. lebedev.ri added a comment. Comment at: clang/docs/ReleaseNotes.rst:238 -- ... +- * ``pointer-overflow`` check was extended added to catch the cases where +a non-zero offset being applied, either to a ``nullptr``, or the resul

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

rsmith added inline comments. Comment at: clang/docs/ReleaseNotes.rst:63 +* As per C++ and C Standards (C++: ``[expr.add]``; C17: 6.5.6p8), applying + non-zero offset to ``nullptr`` (or making non-``nullptr`` a ``nullptr``, In C, even adding 0 to a null pointe

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

lebedev.ri updated this revision to Diff 221929. lebedev.ri edited the summary of this revision. lebedev.ri added a comment. Rebased. I've added most (?) of the obviously-missing folds. This improved situation, although admittedly by not as much as i had hoped. ping @vsk; can this get going plea

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

xbolva00 added a comment. Did you run it with the linux kernel? It could be interesting cc @nickdesaulniers Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67122/new/ https://reviews.llvm.org/D67122 ___

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

rupprecht added a comment. There's definitely a lot of new findings this creates, but it's hard to say exactly how many root causes there are due to the way test failures are (not) grouped well in the way I'm testing. So far they all seem like true positives, so this would be good to submit. Ho

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

lebedev.ri added a comment. In D67122#1663619 , @vsk wrote: > > In D67122#1659721 , @vsk wrote: > > > >> Still think this looks good. Have you tried running this on the llvm test > >> suite, or some other interest

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

vsk added a comment. > In D67122#1659721 , @vsk wrote: > >> Still think this looks good. Have you tried running this on the llvm test >> suite, or some other interesting corpus? Would be curious to see any >> pre/post patch numbers. > > > There were 1.

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

rupprecht added a comment. > But TLDR, either the fix in https://github.com/google/filament/pull/1566 > is incorrect and the actually-bad code is elsewhere, > or you have some other unsanitized UB elsewhere. Could be both :S My money is on "both" :p I tested a random sample of a couple thousan

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

lebedev.ri added a comment. I will still need to see where else `EmitCheckedInBoundsGEP()` should be used, but i'm wondering if it should be best done in follow-ups, since this already catches the interesting bits.. In D67122#1659721 , @vsk wrote: > Sti

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

lebedev.ri updated this revision to Diff 219103. lebedev.ri marked 4 inline comments as done. lebedev.ri added a comment. Rebased over precommitted NFC changes, much nicer diff! :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67122/new/ https://re

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

lebedev.ri marked 5 inline comments as done. lebedev.ri added inline comments. Herald added a subscriber: ychen. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:4703-4720 +// 2) The sign of the difference between the computed address and the base +// pointer matches the si

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

lebedev.ri added a comment. In D67122#1659882 , @rupprecht wrote: > > Still think this looks good. Have you tried running this on the llvm test > > suite, or some other interesting corpus? Would be curious to see any > > pre/post patch numbers. > > I fin

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

rupprecht added a comment. > Still think this looks good. Have you tried running this on the llvm test > suite, or some other interesting corpus? Would be curious to see any pre/post > patch numbers. I finally had time this morning to patch this in and give it a shot. (Sorry for the delay... i

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

vsk added a comment. Still think this looks good. Have you tried running this on the llvm test suite, or some other interesting corpus? Would be curious to see any pre/post patch numbers. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:4703-4720 +// 2) The sign of the diff

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

lebedev.ri added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:4538 + llvm::Value * /*OffsetOverflows*/> +EmitGEPOffsetInBytes(Value *BasePtr, Value *GEPVal, + llvm::LLVMContext &VMContext, CodeGenModule &CGM,

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

lebedev.ri updated this revision to Diff 218792. lebedev.ri marked 25 inline comments as done. lebedev.ri added a comment. @vsk thank you for taking a look! I believe i have addressed or replied to all the comments. Other than the optimization comment, i currently don't believe that is a sound tr

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

vsk added a subscriber: dtzWill. vsk added a comment. Thanks, this is looking pretty good. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:4538 + llvm::Value * /*OffsetOverflows*/> +EmitGEPOffsetInBytes(Value *BasePtr, Value *GEPVal, + llvm::L

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

lebedev.ri marked an inline comment as done. lebedev.ri added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:4703-4720 +// 2) The sign of the difference between the computed address and the base +// pointer matches the sign of the total offset. +llvm:

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

lebedev.ri updated this revision to Diff 218720. lebedev.ri marked 3 inline comments as done. lebedev.ri added a comment. Merged the check into `pointer-overflow`, revisited test coverage. Using `EmitCheckedInBoundsGEP()` in more places TBD. Repository: rG LLVM Github Monorepo CHANGES SINCE L

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

vsk added inline comments. Comment at: clang/docs/UndefinedBehaviorSanitizer.rst:177 ``-fsanitize=undefined``. + - ``-fsanitize=pointer-offsetting``: Enables ``nullptr-and-nonzero-offset`` + and ``pointer-overflow``. lebedev.ri wrote: > vsk wrote: > >

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

lebedev.ri added inline comments. Comment at: clang/docs/UndefinedBehaviorSanitizer.rst:177 ``-fsanitize=undefined``. + - ``-fsanitize=pointer-offsetting``: Enables ``nullptr-and-nonzero-offset`` + and ``pointer-overflow``. vsk wrote: > Why does this

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

lebedev.ri updated this revision to Diff 218556. lebedev.ri marked 7 inline comments as done. lebedev.ri added a comment. @vsk thank you for taking a look! Review notes addressed, except grouping one. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

vsk added inline comments. Comment at: clang/docs/UndefinedBehaviorSanitizer.rst:177 ``-fsanitize=undefined``. + - ``-fsanitize=pointer-offsetting``: Enables ``nullptr-and-nonzero-offset`` + and ``pointer-overflow``. Why does this need to be a separat

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

xbolva00 added inline comments. Comment at: llvm/docs/ReleaseNotes.rst:59 + ``getelementptr inbounds`` can not change the null status of a pointer, + meaning it can not produce non-null pointer given null base pointer, and + likewise given non-null base pointer it can not prod

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

lebedev.ri added inline comments. Comment at: llvm/docs/ReleaseNotes.rst:59 + ``getelementptr inbounds`` can not change the null status of a pointer, + meaning it can not produce non-null pointer given null base pointer, and + likewise given non-null base pointer it can not pr

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

lebedev.ri updated this revision to Diff 218542. lebedev.ri marked 3 inline comments as done. lebedev.ri added a comment. `clang/docs/ReleaseNotes.rst`: also mention C17 verse in addition to C++ verse. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D6

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

xbolva00 added inline comments. Comment at: llvm/docs/ReleaseNotes.rst:59 + ``getelementptr inbounds`` can not change the null status of a pointer, + meaning it can not produce non-null pointer given null base pointer, and + likewise given non-null base pointer it can not prod

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

lebedev.ri marked 2 inline comments as done. lebedev.ri added inline comments. Comment at: llvm/docs/ReleaseNotes.rst:59 + ``getelementptr inbounds`` can not change the null status of a pointer, + meaning it can not produce non-null pointer given null base pointer, and + likew

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

xbolva00 added inline comments. Comment at: llvm/docs/ReleaseNotes.rst:59 + ``getelementptr inbounds`` can not change the null status of a pointer, + meaning it can not produce non-null pointer given null base pointer, and + likewise given non-null base pointer it can not prod

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

lebedev.ri updated this revision to Diff 218533. lebedev.ri marked an inline comment as done. lebedev.ri added a comment. Reworded `(ptr - intptr_t(ptr)) -> nullptr` ubsan message to be less specific. Currently, `EmitCheckedInBoundsGEP()` is used sparsely, a lot of `GEP inbounds` are created dire

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

lebedev.ri updated this revision to Diff 218519. lebedev.ri added a comment. Add test that show that `__builtin_offsetof()` / `((uintptr_t)(&(((S *)nullptr)->y)))` are already not sanitized. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67122/new/

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

xbolva00 added inline comments. Comment at: compiler-rt/test/ubsan/TestCases/Pointer/index-overflow.cpp:15 + // ERR2: runtime error: pointer index expression with base {{.*}} overflowed to + // ERR1: runtime error: subtracting integral value of non-null pointer 0x{{.*}} from

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

aaron.ballman added a comment. In D67122#1656205 , @lebedev.ri wrote: > In D67122#1656189 , @aaron.ballman > wrote: > > > One fear I have with this is in expansions of the `offsetof` macro, where > > it is a comm

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

lebedev.ri added a comment. In D67122#1656189 , @aaron.ballman wrote: > One fear I have with this is in expansions of the `offsetof` macro, where it > is a common implementation strategy to cast a null pointer to be of the > correct type when calculati

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

aaron.ballman added a comment. One fear I have with this is in expansions of the `offsetof` macro, where it is a common implementation strategy to cast a null pointer to be of the correct type when calculating member offsets. Do you think you will be able to distinguish between null pointer ad

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

lebedev.ri created this revision. lebedev.ri added reviewers: vsk, filcab, rsmith, aaron.ballman, vitalybuka, rjmccall, Sanitizers. lebedev.ri added projects: clang, Sanitizers. Herald added subscribers: arphaman, dberris. Herald added a project: LLVM. Quote from http://eel.is/c++draft/expr.add#4

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

lebedev.ri updated this revision to Diff 218507. lebedev.ri added a comment. NFC, fixup docs. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67122/new/ https://reviews.llvm.org/D67122 Files: clang/docs/ReleaseNotes.rst clang/docs/UndefinedBehav