Avoid undefined behavior when doing pointer calculation. #1566

rupprecht · 2019-08-28T16:49:28Z

Performing base + offset pointer arithmetic is only allowed when base itself is not nullptr. In other words, the compiler is assumed to allow that base + offset is always non-null, which an upcoming compiler release will do in this case. The result is that CommandStream.cpp, which calls this in a loop until the result is nullptr, will never terminate (until it runs junk data and crashes).

Avoid this by using intptr_t instead of actual pointers (i.e. char*), which seems to avoid this failure.

Performing `base + offset` pointer arithmetic is only allowed when `base` itself is not nullptr. In other words, the compiler is assumed to allow that `base + offset` is always non-null, which an upcoming compiler release will do in this case. The result is that CommandStream.cpp, which calls this in a loop until the result is nullptr, will never terminate (until it runs junk data and crashes). Avoid this by using intptr_t instead of actual pointers (i.e. char*), which seems to avoid this failure.

romainguy

Thanks! Another reminder that I don't like undefined behaviors :)

pixelflinger · 2019-08-28T18:47:35Z

@rupprecht what if offset is negative? base+offset could be nullptr then (if offset when an int), regardless of base nullness.

pixelflinger · 2019-08-28T18:49:41Z

Oh wait... isn't the reason that this can never be nullptr? rather than "Performing base + offset pointer arithmetic is only allowed when base itself is not nullptr."?

pixelflinger · 2019-08-28T19:00:45Z

@rupprecht honestly I think this is bug in the optimizer of the upcoming compiler. The compiler is allowed to assume that this is not nullptr, I'm okay with that. But I don't see why it would be allowed to assume that this + an_int wouldn't be. You could argue that doing pointer arithmetic on a non nullptr pointer would always be the "this of an object" and therefore cannot be null -- but I'm not buying it because the "this of an object" is not this.

But then you're essentially saying that a pointer can be nullptr only if assigned to nullptr, but can never become nullptr from pointer arithmetic.

Maybe the only true correct solution is to do the equivalent of return base == offset ? nullptr : base + offset, but I'm not willing to add a branch in this code.

rupprecht · 2019-08-28T19:24:07Z

I had the same reaction, and suggested upstream that it might be a miscompile at first, but it's definitely UB (if you're a language lawyer -- which I'm not, but I know a few). I think this is the relevant part of the spec explaining UB for this: http://eel.is/c++draft/expr.add#4

Basically, C++ says that expressions like base + offset are to be interpreted as base pointing to the start of an array, and offset indexes to some other element in that array. It is not allowed to get to nullptr from there (that wouldn't be a valid array member), and therefore the optimizer is allowed to always assume that if (base + offset) is true iff base is non-zero.

We've seen test failures the other way -- code written like if (base + offset) and was returning false when base is nullptr and offset is non-zero. (Explanation: nullptr + is UB per the same link). For that, we implemented a fix similar to what you suggested; replacing base + offset w/ base ? base + offset : reinterpret_cast<const char*>(offset).

Here's the LLVM commit that takes advantage of this (i.e. causes test failures for filament): https://reviews.llvm.org/rL369789

By switching to intptr_t, this follows regular arithmetic rules instead of pointer arithmetic, so you can get to 0 that way.

…defined behaviour Summary: Quote from http://eel.is/c++draft/expr.add#4: ``` 4 When an expression J that has integral type is added to or subtracted from an expression P of pointer type, the result has the type of P. (4.1) If P evaluates to a null pointer value and J evaluates to 0, the result is a null pointer value. (4.2) Otherwise, if P points to an array element i of an array object x with n elements ([dcl.array]), the expressions P + J and J + P (where J has the value j) point to the (possibly-hypothetical) array element i+j of x if 0≤i+j≤n and the expression P - J points to the (possibly-hypothetical) array element i−j of x if 0≤i−j≤n. (4.3) Otherwise, the behavior is undefined. ``` Therefore, as per the standard, applying non-zero offset to `nullptr` (or making non-`nullptr` a `nullptr`, by subtracting pointer's integral value from the pointer itself) is undefined behavior. (*if* `nullptr` is not defined, i.e. e.g. `-fno-delete-null-pointer-checks` was *not* specified.) To make things more fun, in C (6.5.6p8), applying *any* offset to null pointer is undefined, although Clang front-end pessimizes the code by not lowering that info, so this UB is "harmless". Since rL369789 (D66608 `[InstCombine] icmp eq/ne (gep inbounds P, Idx..), null -> icmp eq/ne P, null`) LLVM middle-end uses those guarantees for transformations. If the source contains such UB's, said code may now be miscompiled. Such miscompilations were already observed: * https://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20190826/687838.html * google/filament#1566 Surprisingly, UBSan does not catch those issues ... until now. This diff teaches UBSan about these UB's. `getelementpointer inbounds` is a pretty frequent instruction, so this does have a measurable impact on performance; I've addressed most of the obvious missing folds (and thus decreased the performance impact by ~5%), and then re-performed some performance measurements using my [[ https://github.com/darktable-org/rawspeed | RawSpeed ]] benchmark: (all measurements done with LLVM ToT, the sanitizer never fired.) * no sanitization vs. existing check: average `+21.62%` slowdown * existing check vs. check after this patch: average `22.04%` slowdown * no sanitization vs. this patch: average `48.42%` slowdown Reviewers: vsk, filcab, rsmith, aaron.ballman, vitalybuka, rjmccall, #sanitizers Reviewed By: rsmith Subscribers: kristof.beyls, nickdesaulniers, nikic, ychen, dtzWill, xbolva00, dberris, arphaman, rupprecht, reames, regehr, llvm-commits, cfe-commits Tags: #clang, #sanitizers, #llvm Differential Revision: https://reviews.llvm.org/D67122 llvm-svn: 374293

…defined behaviour Summary: Quote from http://eel.is/c++draft/expr.add#4: ``` 4 When an expression J that has integral type is added to or subtracted from an expression P of pointer type, the result has the type of P. (4.1) If P evaluates to a null pointer value and J evaluates to 0, the result is a null pointer value. (4.2) Otherwise, if P points to an array element i of an array object x with n elements ([dcl.array]), the expressions P + J and J + P (where J has the value j) point to the (possibly-hypothetical) array element i+j of x if 0≤i+j≤n and the expression P - J points to the (possibly-hypothetical) array element i−j of x if 0≤i−j≤n. (4.3) Otherwise, the behavior is undefined. ``` Therefore, as per the standard, applying non-zero offset to `nullptr` (or making non-`nullptr` a `nullptr`, by subtracting pointer's integral value from the pointer itself) is undefined behavior. (*if* `nullptr` is not defined, i.e. e.g. `-fno-delete-null-pointer-checks` was *not* specified.) To make things more fun, in C (6.5.6p8), applying *any* offset to null pointer is undefined, although Clang front-end pessimizes the code by not lowering that info, so this UB is "harmless". Since rL369789 (D66608 `[InstCombine] icmp eq/ne (gep inbounds P, Idx..), null -> icmp eq/ne P, null`) LLVM middle-end uses those guarantees for transformations. If the source contains such UB's, said code may now be miscompiled. Such miscompilations were already observed: * https://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20190826/687838.html * google/filament#1566 Surprisingly, UBSan does not catch those issues ... until now. This diff teaches UBSan about these UB's. `getelementpointer inbounds` is a pretty frequent instruction, so this does have a measurable impact on performance; I've addressed most of the obvious missing folds (and thus decreased the performance impact by ~5%), and then re-performed some performance measurements using my [[ https://github.com/darktable-org/rawspeed | RawSpeed ]] benchmark: (all measurements done with LLVM ToT, the sanitizer never fired.) * no sanitization vs. existing check: average `+21.62%` slowdown * existing check vs. check after this patch: average `22.04%` slowdown * no sanitization vs. this patch: average `48.42%` slowdown Reviewers: vsk, filcab, rsmith, aaron.ballman, vitalybuka, rjmccall, #sanitizers Reviewed By: rsmith Subscribers: kristof.beyls, nickdesaulniers, nikic, ychen, dtzWill, xbolva00, dberris, arphaman, rupprecht, reames, regehr, llvm-commits, cfe-commits Tags: #clang, #sanitizers, #llvm Differential Revision: https://reviews.llvm.org/D67122 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@374293 91177308-0d34-0410-b5e6-96231b3b80d8

…defined behaviour Summary: Quote from http://eel.is/c++draft/expr.add#4: ``` 4 When an expression J that has integral type is added to or subtracted from an expression P of pointer type, the result has the type of P. (4.1) If P evaluates to a null pointer value and J evaluates to 0, the result is a null pointer value. (4.2) Otherwise, if P points to an array element i of an array object x with n elements ([dcl.array]), the expressions P + J and J + P (where J has the value j) point to the (possibly-hypothetical) array element i+j of x if 0≤i+j≤n and the expression P - J points to the (possibly-hypothetical) array element i−j of x if 0≤i−j≤n. (4.3) Otherwise, the behavior is undefined. ``` Therefore, as per the standard, applying non-zero offset to `nullptr` (or making non-`nullptr` a `nullptr`, by subtracting pointer's integral value from the pointer itself) is undefined behavior. (*if* `nullptr` is not defined, i.e. e.g. `-fno-delete-null-pointer-checks` was *not* specified.) To make things more fun, in C (6.5.6p8), applying *any* offset to null pointer is undefined, although Clang front-end pessimizes the code by not lowering that info, so this UB is "harmless". Since rL369789 (D66608 `[InstCombine] icmp eq/ne (gep inbounds P, Idx..), null -> icmp eq/ne P, null`) LLVM middle-end uses those guarantees for transformations. If the source contains such UB's, said code may now be miscompiled. Such miscompilations were already observed: * https://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20190826/687838.html * google/filament#1566 Surprisingly, UBSan does not catch those issues ... until now. This diff teaches UBSan about these UB's. `getelementpointer inbounds` is a pretty frequent instruction, so this does have a measurable impact on performance; I've addressed most of the obvious missing folds (and thus decreased the performance impact by ~5%), and then re-performed some performance measurements using my [[ https://github.com/darktable-org/rawspeed | RawSpeed ]] benchmark: (all measurements done with LLVM ToT, the sanitizer never fired.) * no sanitization vs. existing check: average `+21.62%` slowdown * existing check vs. check after this patch: average `22.04%` slowdown * no sanitization vs. this patch: average `48.42%` slowdown Reviewers: vsk, filcab, rsmith, aaron.ballman, vitalybuka, rjmccall, #sanitizers Reviewed By: rsmith Subscribers: kristof.beyls, nickdesaulniers, nikic, ychen, dtzWill, xbolva00, dberris, arphaman, rupprecht, reames, regehr, llvm-commits, cfe-commits Tags: #clang, #sanitizers, #llvm Differential Revision: https://reviews.llvm.org/D67122 git-svn-id: https://llvm.org/svn/llvm-project/compiler-rt/trunk@374293 91177308-0d34-0410-b5e6-96231b3b80d8

…defined behaviour Summary: Quote from http://eel.is/c++draft/expr.add#4: ``` 4 When an expression J that has integral type is added to or subtracted from an expression P of pointer type, the result has the type of P. (4.1) If P evaluates to a null pointer value and J evaluates to 0, the result is a null pointer value. (4.2) Otherwise, if P points to an array element i of an array object x with n elements ([dcl.array]), the expressions P + J and J + P (where J has the value j) point to the (possibly-hypothetical) array element i+j of x if 0≤i+j≤n and the expression P - J points to the (possibly-hypothetical) array element i−j of x if 0≤i−j≤n. (4.3) Otherwise, the behavior is undefined. ``` Therefore, as per the standard, applying non-zero offset to `nullptr` (or making non-`nullptr` a `nullptr`, by subtracting pointer's integral value from the pointer itself) is undefined behavior. (*if* `nullptr` is not defined, i.e. e.g. `-fno-delete-null-pointer-checks` was *not* specified.) To make things more fun, in C (6.5.6p8), applying *any* offset to null pointer is undefined, although Clang front-end pessimizes the code by not lowering that info, so this UB is "harmless". Since rL369789 (D66608 `[InstCombine] icmp eq/ne (gep inbounds P, Idx..), null -> icmp eq/ne P, null`) LLVM middle-end uses those guarantees for transformations. If the source contains such UB's, said code may now be miscompiled. Such miscompilations were already observed: * https://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20190826/687838.html * google/filament#1566 Surprisingly, UBSan does not catch those issues ... until now. This diff teaches UBSan about these UB's. `getelementpointer inbounds` is a pretty frequent instruction, so this does have a measurable impact on performance; I've addressed most of the obvious missing folds (and thus decreased the performance impact by ~5%), and then re-performed some performance measurements using my [[ https://github.com/darktable-org/rawspeed | RawSpeed ]] benchmark: (all measurements done with LLVM ToT, the sanitizer never fired.) * no sanitization vs. existing check: average `+21.62%` slowdown * existing check vs. check after this patch: average `22.04%` slowdown * no sanitization vs. this patch: average `48.42%` slowdown Reviewers: vsk, filcab, rsmith, aaron.ballman, vitalybuka, rjmccall, #sanitizers Reviewed By: rsmith Subscribers: kristof.beyls, nickdesaulniers, nikic, ychen, dtzWill, xbolva00, dberris, arphaman, rupprecht, reames, regehr, llvm-commits, cfe-commits Tags: #clang, #sanitizers, #llvm Differential Revision: https://reviews.llvm.org/D67122 git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@374293 91177308-0d34-0410-b5e6-96231b3b80d8

…defined behaviour Summary: Quote from http://eel.is/c++draft/expr.add#4: ``` 4 When an expression J that has integral type is added to or subtracted from an expression P of pointer type, the result has the type of P. (4.1) If P evaluates to a null pointer value and J evaluates to 0, the result is a null pointer value. (4.2) Otherwise, if P points to an array element i of an array object x with n elements ([dcl.array]), the expressions P + J and J + P (where J has the value j) point to the (possibly-hypothetical) array element i+j of x if 0≤i+j≤n and the expression P - J points to the (possibly-hypothetical) array element i−j of x if 0≤i−j≤n. (4.3) Otherwise, the behavior is undefined. ``` Therefore, as per the standard, applying non-zero offset to `nullptr` (or making non-`nullptr` a `nullptr`, by subtracting pointer's integral value from the pointer itself) is undefined behavior. (*if* `nullptr` is not defined, i.e. e.g. `-fno-delete-null-pointer-checks` was *not* specified.) To make things more fun, in C (6.5.6p8), applying *any* offset to null pointer is undefined, although Clang front-end pessimizes the code by not lowering that info, so this UB is "harmless". Since rL369789 (D66608 `[InstCombine] icmp eq/ne (gep inbounds P, Idx..), null -> icmp eq/ne P, null`) LLVM middle-end uses those guarantees for transformations. If the source contains such UB's, said code may now be miscompiled. Such miscompilations were already observed: * https://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20190826/687838.html * google/filament#1566 Surprisingly, UBSan does not catch those issues ... until now. This diff teaches UBSan about these UB's. `getelementpointer inbounds` is a pretty frequent instruction, so this does have a measurable impact on performance; I've addressed most of the obvious missing folds (and thus decreased the performance impact by ~5%), and then re-performed some performance measurements using my [[ https://github.com/darktable-org/rawspeed | RawSpeed ]] benchmark: (all measurements done with LLVM ToT, the sanitizer never fired.) * no sanitization vs. existing check: average `+21.62%` slowdown * existing check vs. check after this patch: average `22.04%` slowdown * no sanitization vs. this patch: average `48.42%` slowdown Reviewers: vsk, filcab, rsmith, aaron.ballman, vitalybuka, rjmccall, #sanitizers Reviewed By: rsmith Subscribers: kristof.beyls, nickdesaulniers, nikic, ychen, dtzWill, xbolva00, dberris, arphaman, rupprecht, reames, regehr, llvm-commits, cfe-commits Tags: #clang, #sanitizers, #llvm Differential Revision: https://reviews.llvm.org/D67122 git-svn-id: http://llvm.org/svn/llvm-project/cfe/trunk@374293 91177308-0d34-0410-b5e6-96231b3b80d8

…defined behaviour Summary: Quote from http://eel.is/c++draft/expr.add#4: ``` 4 When an expression J that has integral type is added to or subtracted from an expression P of pointer type, the result has the type of P. (4.1) If P evaluates to a null pointer value and J evaluates to 0, the result is a null pointer value. (4.2) Otherwise, if P points to an array element i of an array object x with n elements ([dcl.array]), the expressions P + J and J + P (where J has the value j) point to the (possibly-hypothetical) array element i+j of x if 0≤i+j≤n and the expression P - J points to the (possibly-hypothetical) array element i−j of x if 0≤i−j≤n. (4.3) Otherwise, the behavior is undefined. ``` Therefore, as per the standard, applying non-zero offset to `nullptr` (or making non-`nullptr` a `nullptr`, by subtracting pointer's integral value from the pointer itself) is undefined behavior. (*if* `nullptr` is not defined, i.e. e.g. `-fno-delete-null-pointer-checks` was *not* specified.) To make things more fun, in C (6.5.6p8), applying *any* offset to null pointer is undefined, although Clang front-end pessimizes the code by not lowering that info, so this UB is "harmless". Since rL369789 (D66608 `[InstCombine] icmp eq/ne (gep inbounds P, Idx..), null -> icmp eq/ne P, null`) LLVM middle-end uses those guarantees for transformations. If the source contains such UB's, said code may now be miscompiled. Such miscompilations were already observed: * https://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20190826/687838.html * google/filament#1566 Surprisingly, UBSan does not catch those issues ... until now. This diff teaches UBSan about these UB's. `getelementpointer inbounds` is a pretty frequent instruction, so this does have a measurable impact on performance; I've addressed most of the obvious missing folds (and thus decreased the performance impact by ~5%), and then re-performed some performance measurements using my [[ https://github.com/darktable-org/rawspeed | RawSpeed ]] benchmark: (all measurements done with LLVM ToT, the sanitizer never fired.) * no sanitization vs. existing check: average `+21.62%` slowdown * existing check vs. check after this patch: average `22.04%` slowdown * no sanitization vs. this patch: average `48.42%` slowdown Reviewers: vsk, filcab, rsmith, aaron.ballman, vitalybuka, rjmccall, #sanitizers Reviewed By: rsmith Subscribers: kristof.beyls, nickdesaulniers, nikic, ychen, dtzWill, xbolva00, dberris, arphaman, rupprecht, reames, regehr, llvm-commits, cfe-commits Tags: #clang, #sanitizers, #llvm Differential Revision: https://reviews.llvm.org/D67122 llvm-svn: 374293

…ro-offset Summary: Per standard pointer arithmetic is only defined when keeping pointers within the bounds of an array object. Applying non-zero offset to nullptr (or making non-nullptr a nullptr by subtracting pointer's integral value from the pointer itself) is undefined behavior. Since https://reviews.llvm.org/D66608 `[InstCombine] icmp eq/ne (gep inbounds P, Idx..), null -> icmp eq/ne P, null) LLVM middle-end uses those guarantees for transformations.` and mis-compilations have been observed: - https://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20190826/687838.html - google/filament#1566 To prevent help weed out bugs before they lead to future miscompilations a new UBSAN check has been added: `nullptr-with-nonzero-offset` - https://reviews.llvm.org/D67122 [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour `folly::io::Cursor` does this type of operations when checking if `crtPos_ + N <= crtEnd_`: when it's empty it becomes `nullptr + N <= nullptr`: const uint8_t* crtBegin_{nullptr}; const uint8_t* crtEnd_{nullptr}; const uint8_t* crtPos_{nullptr}; Switch to `uintptr_t` space where the math is no longer UB. Reviewed By: yfeldblum Differential Revision: D33737556 fbshipit-source-id: 588b91ac1387112a6f183edfda5555ca1b7193d8

…NULL (caught by UBSAN nullptr-with-nonzero-offset) According to the standard pointer arithmetic is only defined in these situations: - `nullptr` ± 0 => `nullptr` - P±N and N±P are pointers that point at an element of the same array with index I±N `**nullptr**`** + N≠0 is undefined behavior** - even if you don't dereference the resulting pointer. Since [InstCombine] icmp eq/ne (gep inbounds P, Idx..), null -> icmp eq/ne P, null) https://reviews.llvm.org/D66608 LLVM middle-end uses those guarantees for transformations and mis-compilations have been observed: - https://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20190826/687838.html - google/filament#1566 To prevent help weed out bugs before they lead to future miscompilations a new UBSAN check has been added: `nullptr-with-nonzero-offset` - [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour https://reviews.llvm.org/D67122 This new nullptr-with-nonzero-offset check fails on invocations of this macro. Fix the macro to avoid adding to NULL.

googlebot added the cla: yes label Aug 28, 2019

romainguy reviewed Aug 28, 2019

View reviewed changes

romainguy assigned pixelflinger Aug 28, 2019

pixelflinger approved these changes Aug 28, 2019

View reviewed changes

pixelflinger merged commit 0ad21cd into google:master Aug 28, 2019

luciang mentioned this pull request Apr 8, 2022

__Pyx_KwValues_FASTCALL: avoid triggering UB by adding non-zero to NULL (caught by UBSAN nullptr-with-nonzero-offset) cython/cython#4726

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid undefined behavior when doing pointer calculation. #1566

Avoid undefined behavior when doing pointer calculation. #1566

rupprecht commented Aug 28, 2019

romainguy left a comment

pixelflinger commented Aug 28, 2019

pixelflinger commented Aug 28, 2019

pixelflinger commented Aug 28, 2019 •

edited

rupprecht commented Aug 28, 2019

Avoid undefined behavior when doing pointer calculation. #1566

Avoid undefined behavior when doing pointer calculation. #1566

Conversation

rupprecht commented Aug 28, 2019

romainguy left a comment

Choose a reason for hiding this comment

pixelflinger commented Aug 28, 2019

pixelflinger commented Aug 28, 2019

pixelflinger commented Aug 28, 2019 • edited

rupprecht commented Aug 28, 2019

pixelflinger commented Aug 28, 2019 •

edited