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 <mpe...@apache.org> 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 <mpe...@apache.org> 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
> >> <gsolov...@cloudera.com.invalid>
> 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
> <a...@cloudera.com.invalid>
> >> 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 <aw...@apache.org> 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
> >> >
>
