Another option is instead of trying to fix RapidJSON for 1.9.0, we can just relnote that the affected OSes / compilers are not thoroughly tested and Kudu may have issues with parsing some JSON content if compiled in those environments. After all, that code has apparently been there for years so it’s not a regression.
I’ll have more time to look into this on Sunday and during the week. Mike > On Mar 1, 2019, at 6:07 PM, Mike Percy <[email protected]> wrote: > > I'm also seeing the jsonreader-test crash in RELEASE mode on Ubuntu bionic. > The symptom is a nullptr dereference but really it's caused by an assertion > failure. > > I modified the test to get a proper failure and it fails here: > > // Min signed 32-bit integer. > const char* const signed_small32 = "signed_small32"; > Status s = r.ExtractUint32(r.root(), signed_small32, &dummy_u32); > ASSERT_TRUE(s.IsInvalidArgument()) << s.ToString(); > > That status object is OK, which means jsonreader incorrectly detected the > negative integer as a uint in the following piece of code: > > Status JsonReader::ExtractUint32(const Value* object, > const char* field, > uint32_t* result) const { > const Value* val; > RETURN_NOT_OK(ExtractField(object, field, &val)); > if (PREDICT_FALSE(!val->IsUint())) { > return Status::InvalidArgument(Substitute( > "wrong type during field extraction: expected uint32 but got $0", > TypeToString(val->GetType()))); > } > *result = val->GetUint(); > return Status::OK(); > } > > Note that this only happens in RELEASE mode for me. In this test, there is a > comment preventing UB testing that looks like this: > > // The rapidjson code has some improper handling of the min int32 and min > // int64 that exposes UB. > #if defined(ADDRESS_SANITIZER) > LOG(WARNING) << "this test is skipped in ASAN builds"; > return; > #endif > > It's surprising we have never noticed this before. Maybe only the newer > compilers are causing problems due to the undefined behavior. > > When I comment out that #ifdef and run this under ASAN with UBSAN enabled, I > get the following warning: > > [ RUN ] JsonReaderTest.SignedAndUnsignedInts > thirdparty/installed/common/include/rapidjson/reader.h:644:18: runtime error: > negation of -2147483648 cannot be represented in type 'int'; cast to an > unsigned type to negate this value to itself > #0 0x8b15e4 in void rapidjson::GenericReader<rapidjson::UTF8<char>, > rapidjson::MemoryPoolAllocator<rapidjson::CrtAllocator> >::ParseNumber<0u, > rapidjson::GenericStringStream<rapidjson::UTF8<char> >, > rapidjson::GenericDocument<rapidjson::UTF8<char>, > rapidjson::MemoryPoolAllocator<rapidjson::CrtAllocator> > > >(rapidjson::GenericStringStream<rapidjson::UTF8<char> >&, > rapidjson::GenericDocument<rapidjson::UTF8<char>, > rapidjson::MemoryPoolAllocator<rapidjson::CrtAllocator> >&) > ../thirdparty/installed/common/include/rapidjson/reader.h:644:18 > #1 0x8aaf00 in void rapidjson::GenericReader<rapidjson::UTF8<char>, > rapidjson::MemoryPoolAllocator<rapidjson::CrtAllocator> >::ParseValue<0u, > rapidjson::GenericStringStream<rapidjson::UTF8<char> >, > rapidjson::GenericDocument<rapidjson::UTF8<char>, > rapidjson::MemoryPoolAllocator<rapidjson::CrtAllocator> > > >(rapidjson::GenericStringStream<rapidjson::UTF8<char> >&, > rapidjson::GenericDocument<rapidjson::UTF8<char>, > rapidjson::MemoryPoolAllocator<rapidjson::CrtAllocator> >&) > ../thirdparty/installed/common/include/rapidjson/reader.h:663:14 > #2 0x8a8b96 in void rapidjson::GenericReader<rapidjson::UTF8<char>, > rapidjson::MemoryPoolAllocator<rapidjson::CrtAllocator> >::ParseObject<0u, > rapidjson::GenericStringStream<rapidjson::UTF8<char> >, > rapidjson::GenericDocument<rapidjson::UTF8<char>, > rapidjson::MemoryPoolAllocator<rapidjson::CrtAllocator> > > >(rapidjson::GenericStringStream<rapidjson::UTF8<char> >&, > rapidjson::GenericDocument<rapidjson::UTF8<char>, > rapidjson::MemoryPoolAllocator<rapidjson::CrtAllocator> >&) > ../thirdparty/installed/common/include/rapidjson/reader.h:290:4 > #3 0x8a7829 in bool rapidjson::GenericReader<rapidjson::UTF8<char>, > rapidjson::MemoryPoolAllocator<rapidjson::CrtAllocator> >::Parse<0u, > rapidjson::GenericStringStream<rapidjson::UTF8<char> >, > rapidjson::GenericDocument<rapidjson::UTF8<char>, > rapidjson::MemoryPoolAllocator<rapidjson::CrtAllocator> > > >(rapidjson::GenericStringStream<rapidjson::UTF8<char> >&, > rapidjson::GenericDocument<rapidjson::UTF8<char>, > rapidjson::MemoryPoolAllocator<rapidjson::CrtAllocator> >&) > ../thirdparty/installed/common/include/rapidjson/reader.h:243:15 > #4 0x8a6f10 in rapidjson::GenericDocument<rapidjson::UTF8<char>, > rapidjson::MemoryPoolAllocator<rapidjson::CrtAllocator> >& > rapidjson::GenericDocument<rapidjson::UTF8<char>, > rapidjson::MemoryPoolAllocator<rapidjson::CrtAllocator> >::ParseStream<0u, > rapidjson::GenericStringStream<rapidjson::UTF8<char> > > >(rapidjson::GenericStringStream<rapidjson::UTF8<char> >&) > ../thirdparty/installed/common/include/rapidjson/document.h:712:23 > #5 0x8a34f2 in rapidjson::GenericDocument<rapidjson::UTF8<char>, > rapidjson::MemoryPoolAllocator<rapidjson::CrtAllocator> >& > rapidjson::GenericDocument<rapidjson::UTF8<char>, > rapidjson::MemoryPoolAllocator<rapidjson::CrtAllocator> >::Parse<0u>(char > const*) ../thirdparty/installed/common/include/rapidjson/document.h:745:10 > #6 0x89d804 in kudu::JsonReader::Init() > ../src/kudu/util/jsonreader.cc:65:13 > #7 0x7754f7 in > kudu::JsonReaderTest_SignedAndUnsignedInts_Test::TestBody() > ../src/kudu/util/jsonreader-test.cc:172:3 > #8 0xe69c6c in void > testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, > void>(testing::Test*, void (testing::Test::*)(), char const*) > /home/mpercy/src/kudu/thirdparty/src/googletest-release-1.8.0/googletest/src/gtest.cc:2402 > #9 0xe69c6c in void > testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, > void>(testing::Test*, void (testing::Test::*)(), char const*) > /home/mpercy/src/kudu/thirdparty/src/googletest-release-1.8.0/googletest/src/gtest.cc:2438 > #10 0xe60391 in testing::Test::Run() > /home/mpercy/src/kudu/thirdparty/src/googletest-release-1.8.0/googletest/src/gtest.cc:2474 > #11 0xe60473 in testing::TestInfo::Run() > /home/mpercy/src/kudu/thirdparty/src/googletest-release-1.8.0/googletest/src/gtest.cc:2656 > #12 0xe605b6 in testing::TestCase::Run() > /home/mpercy/src/kudu/thirdparty/src/googletest-release-1.8.0/googletest/src/gtest.cc:2774 > #13 0xe60a87 in testing::internal::UnitTestImpl::RunAllTests() > /home/mpercy/src/kudu/thirdparty/src/googletest-release-1.8.0/googletest/src/gtest.cc:4649 > #14 0xe6a14c in bool > testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, > bool>(testing::internal::UnitTestImpl*, bool > (testing::internal::UnitTestImpl::*)(), char const*) > /home/mpercy/src/kudu/thirdparty/src/googletest-release-1.8.0/googletest/src/gtest.cc:2402 > #15 0xe6a14c in bool > testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, > bool>(testing::internal::UnitTestImpl*, bool > (testing::internal::UnitTestImpl::*)(), char const*) > /home/mpercy/src/kudu/thirdparty/src/googletest-release-1.8.0/googletest/src/gtest.cc:2438 > #16 0xe60be0 in testing::UnitTest::Run() > /home/mpercy/src/kudu/thirdparty/src/googletest-release-1.8.0/googletest/src/gtest.cc:4257 > #17 0x7a61bb in RUN_ALL_TESTS() > ../thirdparty/installed/uninstrumented/include/gtest/gtest.h:2233:46 > #18 0x7a2b24 in main ../src/kudu/util/test_main.cc:106:13 > #19 0x7f36f9ffeb96 in __libc_start_main csu/libc-start.c:310 > #20 0x663409 in _start > (/home/mpercy/src/kudu/build/asandebug/bin/jsonreader-test+0x663409) > > SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior > thirdparty/installed/common/include/rapidjson/reader.h:644:18 in > > When I rebased on top of this patch to upgrade RapidJSON to 1.1.0, the test > passed and the crash did not occur: https://gerrit.cloudera.org/c/12643/ > > Looking at the RapidJSON change log for the 1.1.0 release, they mention > fixing the undefined behavior > <http://rapidjson.org/md__c_h_a_n_g_e_l_o_g.html>: > > 1.1.0 - 2016-08-25 > Added > * Fix undefined behaviour (#646) which references > https://github.com/Tencent/rapidjson/pull/646/commits ... I am not sure if > the actual relevant bugfix is in that PR or not but it seems that PR finally > got RapidJSON passing the UBSAN tests. > > I think we should run another RC since the UB is actually causing observable > undefined / incorrect behavior with new compiler optimizations, at least in a > test. > > Mike > > >> On Fri, Mar 1, 2019 at 3:40 PM Greg Solovyev >> <[email protected]> wrote: >> +1 >> I built on Ubuntu 16, Ubuntu 18 and OSX 10.14.3. I also tested Docker >> build. >> >> The following test failed consistently on release and debug builds on >> Ubuntu 16: >> >> - client_examples-test >> >> The following test was flaky on Ubuntu 16: >> >> - authn_token_expire-itest >> >> The following test failed consistently on release build on Ubuntu18: >> >> - jsonreader-test >> >> Docker build took about 2 hours after increasing Docker RAM to 6GB. The >> first 3 attempts failed when I had Docker RAM set to 4.2GB. >> >> On OSX 10.14.3 the following tests failed on release build: >> 23 - hybrid_clock-test (Failed) >> 175 - sentry_authz_provider-test (Failed) >> 197 - sentry_client-test (Failed) >> 261 - kudu-tool-test.2 (Failed) >> 313 - jsonreader-test (Failed) >> >> Greg >> >> On Wed, Feb 27, 2019 at 6:39 PM Adar Lieber-Dembo <[email protected]> >> wrote: >> >> > +1 >> > >> > I ran the C++ tests (DEBUG build) in slow mode on Ubuntu 18, CentOS >> > 6.6, and CentOS 7.3. I saw a couple failures but I'm pretty sure >> > they're just flakes; filed KUDU-2718 and KUDU-2719 for them. >> > >> > >> > On Wed, Feb 27, 2019 at 1:51 AM Andrew Wong <[email protected]> wrote: >> > > >> > > Hello Kudu devs! >> > > >> > > As suggested on the RC1 voting thread, I've included the fix for >> > KUDU-2710 >> > > and some other bug fixes that landed on master since RC1, and created a >> > new >> > > release candidate for Apache Kudu 1.9.0. >> > > >> > > Apache Kudu 1.9.0 is a minor release that offers many improvements and >> > > fixes since the prior release. >> > > >> > > This is a source-only release. The artifacts have been staged here: >> > > https://dist.apache.org/repos/dist/dev/kudu/1.9.0-RC2/ >> > > >> > > Java convenience binaries in the form of a Maven repository are staged >> > here: >> > > https://repository.apache.org/content/repositories/orgapachekudu-1029/ >> > > >> > > It is tagged in Git as 1.9.0-RC2 and the corresponding hash is the >> > > following: >> > > >> > https://gitbox.apache.org/repos/asf?p=kudu.git;a=commit;h=deb0f04b010c5399771879829285c284b8f20008 >> > > >> > > A draft of the release notes can be found here: >> > > https://gerrit.cloudera.org/c/12389/ >> > > >> > > The KEYS file to verify the artifact signatures can be found here: >> > > https://dist.apache.org/repos/dist/release/kudu/KEYS >> > > >> > > I'd suggest going through the README and the release notes, building >> > Kudu, >> > > and >> > > running the unit tests. Testing out the Maven repo would also be >> > > appreciated. >> > > >> > > The vote will run until this coming Friday, March 1st at 11:59AM PST. >> > > >> > > >> > > Thank you, >> > > Andrew >> >
