Thank you all for participating thus far! I'm inclined to agree with just release noting this at best (reminder, the release notes are still up as a draft here <https://gerrit.cloudera.org/c/12389/>), given the limited places we use RapidJSON, and the fact that this isn't a regression in 1.9.
I'll hold off until Monday to tally the results in case there are further findings. On Fri, Mar 1, 2019 at 11:36 PM Mike Percy <[email protected]> wrote: > 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 > >> > >
