[PATCH] D91258: [clangd] Sanity-check array sizes read from disk before allocating them.

Fri Nov 13 03:16:44 PST 2020

sammccall added a comment.

In D91258#2392646 <https://reviews.llvm.org/D91258#2392646>, @eugenis wrote:

> Hi Sam,
>
> this patch is failing on the ubsan bot with:
>
>   [ RUN      ] SerializationTest.NoCrashOnBadArraySize
>   /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang-tools-extra/clangd/index/Serialization.cpp:90:26: runtime error: left shift of 127 by 28 places cannot be represented in type 'int'
>       #0 0x392dd57 in clang::clangd::(anonymous namespace)::Reader::consumeVar() /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang-tools-extra/clangd/index/Serialization.cpp:90:26

Thanks Evgenii! (It's really surprising to me this is using signed arithmetic!)

Is the sanitizer strictly correct here that this is UB? From my reading only C requires the result of signed shifts to be representable, not C++. https://eel.is/c++draft/expr.shift
So this should be well-defined with the same result as the unsigned arithmetic (assuming 2s complement, I guess).

(We'll fix this to use unsigned arithmetic in any case)

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91258/new/

https://reviews.llvm.org/D91258