Page MenuHomePhabricator

Syndicate, test and fix base64 implementation
ClosedPublic

Authored by serge-sans-paille on Feb 24 2020, 8:28 AM.

Details

Summary

As an answer to https://github.com/llvm/llvm-project/issues/149, and to a comment in the code, syndicate base64 implementation between compiler-rt and clang-tools-extra

Diff Detail

Event Timeline

Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptFeb 24 2020, 8:28 AM
Herald added subscribers: llvm-commits, Restricted Project, cfe-commits and 5 others. · View Herald Transcript
lebedev.ri requested changes to this revision.Feb 24 2020, 8:40 AM
lebedev.ri added a subscriber: lebedev.ri.
lebedev.ri added inline comments.
compiler-rt/lib/fuzzer/FuzzerUtil.cpp
14–16

I don't believe we can do this. libfuzzer is acting as a sub-sub-project,
and i strongly believe it wants to shy away from using anything
other than it's own headers and standard c/c++ headers.

This revision now requires changes to proceed.Feb 24 2020, 8:40 AM

Restore duplication, fix the bug in two separate locations.

lebedev.ri resigned from this revision.Feb 24 2020, 10:06 AM

As a side note, pre-reserving the buffer size gives interesting speedup, see http://quick-bench.com/Lp6OwO2etW1YEmJayVWn3U8JDiY

Other copy/paste typo.

thanks for doing this! didn't look into details yet. Could you explain what was the bug in the previous code? since this patch contains some refactoring changes, it is not quite straightforward to spot it.

llvm/unittests/Support/Base64Test.cpp
2

nit: Base64Test.cpp.

31

nit: i would just inline the TestBase64.

thanks for doing this! didn't look into details yet. Could you explain what was the bug in the previous code?

Yeah, the code was using + operator instead of | to combine the results, which is a problem when shifting signed values. (0xFF << 16) is implicitly converted to a (signed) int, and thus results in 0xffff0000, which is negative. Combining negative numbers with a + in that context is not what we want to do.

Add a test case that exhibits the overflow in previous implementation + minor nits from @hokein

serge-sans-paille marked 3 inline comments as done.Feb 27 2020, 11:14 AM
serge-sans-paille added inline comments.
llvm/unittests/Support/Base64Test.cpp
31

I wouldn't :-) I find it easier to read like that, if that's okay with you.

hokein accepted this revision.Feb 28 2020, 3:46 AM

thanks for doing this! didn't look into details yet. Could you explain what was the bug in the previous code?

Yeah, the code was using + operator instead of | to combine the results, which is a problem when shifting signed values. (0xFF << 16) is implicitly converted to a (signed) int, and thus results in 0xffff0000, which is negative. Combining negative numbers with a + in that context is not what we want to do.

fair enough, thanks for the clarification.

llvm/unittests/Support/Base64Test.cpp
31

personal I don't think it is easier comparing with inline them, EXPECT_EQ(encodeBase64("f"), "abc"); seems clearer to me, up to you.

43

nit: explicit StringRef is not needed.

This revision is now accepted and ready to land.Feb 28 2020, 3:46 AM
This revision was automatically updated to reflect the committed changes.
hctim added a subscriber: hctim.Mar 2 2020, 9:31 AM

Looks like this change broke the UBSan build bots:

FAIL: LLVM-Unit :: Support/./SupportTests/Base64Test.Base64 (3484 of 36324)
******************** TEST 'LLVM-Unit :: Support/./SupportTests/Base64Test.Base64' FAILED ********************
Note: Google Test filter = Base64Test.Base64
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from Base64Test
[ RUN      ] Base64Test.Base64
/b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/include/llvm/Support/Base64.h:42:28: runtime error: left shift of negative value -1
    #0 0x4993b6 in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > llvm::encodeBase64<llvm::StringRef>(llvm::StringRef const&) /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/include/llvm/Support/Base64.h:42:28
    #1 0x498b78 in (anonymous namespace)::TestBase64(llvm::StringRef, llvm::StringRef) /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/unittests/Support/Base64Test.cpp:23:14
    #2 0x498aeb in Base64Test_Base64_Test::TestBody() /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/unittests/Support/Base64Test.cpp:42:3
    #3 0x8d85f9 in testing::Test::Run() /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:2474:5
    #4 0x8d9581 in testing::TestInfo::Run() /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:2656:11
    #5 0x8d9f32 in testing::TestCase::Run() /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:2774:28
    #6 0x8e1602 in testing::internal::UnitTestImpl::RunAllTests() /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:4649:43
    #7 0x8e1085 in testing::UnitTest::Run() /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:4257:10
    #8 0x8d0933 in RUN_ALL_TESTS /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/utils/unittest/googletest/include/gtest/gtest.h:2233:46
    #9 0x8d0933 in main /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/utils/unittest/UnitTestMain/TestMain.cpp:50:10
    #10 0x7f806f76e2e0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x202e0)
    #11 0x445d39 in _start (/b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm_build_ubsan/unittests/Support/SupportTests+0x445d39)

SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/include/llvm/Support/Base64.h:42:28 in 

********************
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90.. 

Testing Time: 68.95s
********************
Failing Tests (1):
    LLVM-Unit :: Support/./SupportTests/Base64Test.Base64

I'll send out a revert shortly, to reproduce the sanitizer bots you can use the instructions here, or, given that this is UBSan, it should be sufficient to just add -DLLVM_USE_SANITIZER=Undefined and run check-llvm :).