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

Thu Oct 10 05:51:07 PDT 2019

On Thu, Oct 10, 2019 at 3:45 PM Rui Ueyama <ruiu at google.com> wrote:
>
> I think I now understand what is the problem. Submitted a patch on top of this as r374332.
Yes, i'm not really confident in my 'fix'. Thank you for the adjustment!

> On Thu, Oct 10, 2019 at 9:29 PM Rui Ueyama <ruiu at google.com> wrote:
>>
>> I'm a little confused. isecLoc is initialized in the following statement
>>
>> uint8_t *isecLoc = Out::bufferStart + isec->getParent()->offset + isec->outSecOff;
>>
>> , and I don't know how isecLoc can become a nullptr. If Out::bufferStart is a nullptr, the initialization expression should trigger a UBsan.
>>
>> On Thu, Oct 10, 2019 at 9:26 PM Rui Ueyama <ruiu at google.com> wrote:
>>>
>>> I guess that you are fixing UB issues that was not noticed before but can now be caught by an improved UBsan?
Yes, absolutely.
This is https://reviews.llvm.org/D67122 which landed in r374293

>>> 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...
Yes, i'm sorry, i would have absolutely submitted this as a review,
but sadly i haven't had much luck
with ironing-out these stage2 bugs before landing because it just is
too slow for me to do locally.

Roman

>>> 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