[lld] r374329 - [lld] getErrPlace(): don't perform arithmetics on maybe-null pointer

Thu Oct 10 05:26:37 PDT 2019

I guess that you are fixing UB issues that was not noticed before but can
now be caught by an improved UBsan? I hoped that you would sent it for
review, but if you are trying to fix buildbots, I can see why you wanted to
submit this...

On Thu, Oct 10, 2019 at 9:20 PM Roman Lebedev via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> Author: lebedevri
> Date: Thu Oct 10 05:22:55 2019
> New Revision: 374329
>
> URL: http://llvm.org/viewvc/llvm-project?rev=374329&view=rev
> Log:
> [lld] getErrPlace(): don't perform arithmetics on maybe-null pointer
>
> isecLoc there can be null, but at the same time isec->getSize() may
> be non-null. It is UB to offset a nullptr.The most straight-forward fix
> here appears to perform casts+normal integral arithmetics.
>
> FAIL: lld :: ELF/invalid/invalid-relocation-aarch64.test (1158 of 2217)
> ******************** TEST 'lld ::
> ELF/invalid/invalid-relocation-aarch64.test' FAILED ********************
> Script:
> --
> : 'RUN: at line 2';
>  /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm_build_ubsan/bin/yaml2obj
> /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/lld/test/ELF/invalid/invalid-relocation-aarch64.test
> -o
> /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm_build_ubsan/tools/lld/test/ELF/invalid/Output/invalid-relocation-aarch64.test.tmp.o
> : 'RUN: at line 3';   not
> /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm_build_ubsan/bin/ld.lld
> /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm_build_ubsan/tools/lld/test/ELF/invalid/Output/invalid-relocation-aarch64.test.tmp.o
> -o /dev/null 2>&1 |
> /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm_build_ubsan/bin/FileCheck
> /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/lld/test/ELF/invalid/invalid-relocation-aarch64.test
> --
> Exit Code: 1
>
> Command Output (stderr):
> --
> /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/lld/test/ELF/invalid/invalid-relocation-aarch64.test:4:10:
> error: CHECK: expected string not found in input
> # CHECK: error: unknown relocation (1024) against symbol foo
>          ^
> <stdin>:1:1: note: scanning from here
> /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/lld/ELF/Target.cpp:100:41:
> runtime error: applying non-zero offset 24 to null pointer
> ^
> <stdin>:1:118: note: possible intended match here
> /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/lld/ELF/Target.cpp:100:41:
> runtime error: applying non-zero offset 24 to null pointer
>
>                                            ^
>
> --
>
> ********************
> Testing:  0.. 10.. 20.. 30.. 40.. 50.
> FAIL: lld :: ELF/invalid/invalid-relocation-x64.test (1270 of 2217)
> ******************** TEST 'lld :: ELF/invalid/invalid-relocation-x64.test'
> FAILED ********************
> Script:
> --
> : 'RUN: at line 2';
>  /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm_build_ubsan/bin/yaml2obj
> /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/lld/test/ELF/invalid/invalid-relocation-x64.test
> -o
> /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm_build_ubsan/tools/lld/test/ELF/invalid/Output/invalid-relocation-x64.test.tmp1.o
> : 'RUN: at line 3';   echo ".global foo; foo:" >
> /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm_build_ubsan/tools/lld/test/ELF/invalid/Output/invalid-relocation-x64.test.tmp2.s
> : 'RUN: at line 4';
>  /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm_build_ubsan/bin/llvm-mc
> /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm_build_ubsan/tools/lld/test/ELF/invalid/Output/invalid-relocation-x64.test.tmp2.s
> -o
> /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm_build_ubsan/tools/lld/test/ELF/invalid/Output/invalid-relocation-x64.test.tmp2.o
> -filetype=obj -triple x86_64-pc-linux
> : 'RUN: at line 5';   not
> /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm_build_ubsan/bin/ld.lld
> /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm_build_ubsan/tools/lld/test/ELF/invalid/Output/invalid-relocation-x64.test.tmp1.o
> /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm_build_ubsan/tools/lld/test/ELF/invalid/Output/invalid-relocation-x64.test.tmp2.o
> -o /dev/null 2>&1 |
> /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm_build_ubsan/bin/FileCheck
> /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/lld/test/ELF/invalid/invalid-relocation-x64.test
> --
> Exit Code: 1
>
> Command Output (stderr):
> --
> /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/lld/test/ELF/invalid/invalid-relocation-x64.test:6:10:
> error: CHECK: expected string not found in input
> # CHECK: error: unknown relocation (152) against symbol foo
>          ^
> <stdin>:1:1: note: scanning from here
> /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/lld/ELF/Target.cpp:100:41:
> runtime error: applying non-zero offset 24 to null pointer
> ^
> <stdin>:1:118: note: possible intended match here
> /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/lld/ELF/Target.cpp:100:41:
> runtime error: applying non-zero offset 24 to null pointer
>
>                                            ^
>
> --
>
> ********************
> Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90..
> Testing Time: 20.73s
> ********************
> Failing Tests (2):
>     lld :: ELF/invalid/invalid-relocation-aarch64.test
>     lld :: ELF/invalid/invalid-relocation-x64.test
>
> Modified:
>     lld/trunk/ELF/Target.cpp
>
> Modified: lld/trunk/ELF/Target.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/Target.cpp?rev=374329&r1=374328&r2=374329&view=diff
>
> ==============================================================================
> --- lld/trunk/ELF/Target.cpp (original)
> +++ lld/trunk/ELF/Target.cpp Thu Oct 10 05:22:55 2019
> @@ -97,7 +97,12 @@ template <class ELFT> static ErrorPlace
>        continue;
>
>      uint8_t *isecLoc = Out::bufferStart + isec->getParent()->offset +
> isec->outSecOff;
> -    if (isecLoc <= loc && loc < isecLoc + isec->getSize())
> +    if (isecLoc > loc)
> +      continue;
> +    // isecLoc might be nullptr here, with isec->getSize() being non-zero.
> +    // Adding these two together is not defined in C++.
> +    if (loc < reinterpret_cast<uint8_t *>(
> +                  reinterpret_cast<std::uintptr_t>(isecLoc) +
> isec->getSize()))
>

This new expression doesn't make much sense to me. Perhaps we should do
`continue` if isecLoc is null.

       return {isec, isec->template getLocation<ELFT>(loc - isecLoc) + ":
> "};
>    }
>    return {};
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20191010/7f0acca1/attachment.html>