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
> > >> >
> >
