[PATCH] D63518: BitStream reader: propagate errors

[JF Bastien via Phabricator via cfe-commits]

jfb marked 2 inline comments as done.
jfb added inline comments.


================
Comment at: clang-tools-extra/trunk/clang-doc/BitcodeReader.cpp:619
+    // FIXME check that the enum is in range.
+    auto Code = static_cast<llvm::bitc::FixedAbbrevIDs>(MaybeCode.get());
+
----------------
bjope wrote:
> This has caused problem for our sanitizer tests the last couple of days:
> 
> ```
> FAIL: Extra Tools Unit Tests :: clang-doc/./ClangDocTests/BitcodeTest.emitMethodInfoBitcode (20077 of 48746)
> ******************** TEST 'Extra Tools Unit Tests :: clang-doc/./ClangDocTests/BitcodeTest.emitMethodInfoBitcode' FAILED ********************
> Note: Google Test filter = BitcodeTest.emitMethodInfoBitcode
> [==========] Running 1 test from 1 test case.
> [----------] Global test environment set-up.
> [----------] 1 test from BitcodeTest
> [ RUN      ] BitcodeTest.emitMethodInfoBitcode
> /local/repo/bbiswjenk/fem023-eiffel003/workspace/llvm/llvm-master-sanitize/clang-tools-extra/clang-doc/BitcodeReader.cpp:621:13: runtime error: load of value 9, which is not a valid value for type 'llvm::bitc::FixedAbbrevIDs'
> 
> ```
> 
> This was seen when building trunk with clang 6.0.0 and LVM_USE_SANITIZER=Undefined
> 
> ```
> cmake -G Ninja '-DLLVM_ENABLE_PROJECTS='\''clang;clang-tools-extra'\''' -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=ON '-DCMAKE_CXX_FLAGS='\''-stdlib=libc++'\''' -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DLLVM_USE_LINKER=gold -DLLVM_USE_SANITIZER=Undefined ../.
> -- The C compiler identification is Clang 6.0.0
> -- The CXX compiler identification is Clang 6.0.0
> ```
> 
> Afaict we can't cast the read value to FixedAbbrevIDs as we do not know yet if it matches one of the values defined in the enum, or if we will take the default case.
> 
> 
> A similar switch exist at cfe/trunk/lib/Frontend/SerializedDiagnosticReader.cpp:127, but it is using a slightly different pattern:
> 
> ```
> unsigned Code;
> Code = Res.get();
> switch ((llvm::bitc::FixedAbbrevIDs)Code) 
> ```
> I haven't seen any failures for SerializedDiagnosticReader. So either we lack test coverage for that function, or the sanitizer only introduce the check when using the static_cast (and declaring Code as an enum) as done here.
> 
That sounds like a pre-existing bug. We should check that the value is in range before casting. Can you send patches to fix both code locations, and add test coverage? This code is indeed poorly tested.

Why do the sanitizers catch `static_cast` but not C-style casts?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63518