+1, adding a note about this to the known issues section makes sense. I didn't have time to investigate in detail today, but it sounds like the issue only affects min int32 and min int64. I can verify that tomorrow.
Mike On Sat, Mar 2, 2019 at 12:30 AM Attila Bukor <[email protected]> wrote: > Sounds fair to me as well. I don't think the release notes are the right > place to document this though, maybe to known issues. Adding it to the > release notes would suggest it's a regression. > On Sat, Mar 02, 2019 at 12:03:34AM -0800, Andrew Wong wrote: > > 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 > > > >> > > > > >
