Json::Value::null is constructed at random time during program initialization, and can cause segfault

[marklakata]

The Json::Value::null object is a global reference object. The rules of C++ allow other constructors from other translation units to access the Json::Value::null symbol before it is constructed, which means that the reference is bogus, and you get a segmentation violation for accessing it. That means if you have a global object that uses Json::Value in its constructor, it can easily crash, depending on the whim of the linker deciding what to link first.

The solution (which I have shown works in my private workspace) is to replace all instances of Json::Value::null with static function (say Json::Value::null_()) in the style of a Meyers Singleton. Then accesses to null_() will always be valid and not depend on a race condition of the global construction list.

const Value& Value::null_()   
{
 static const Json::Value nullStatic(nullValue);
 return nullStatic;
}

// for backwards compatibility, we'll leave this global references around, but DO NOT 
// use them in JSONCPP library code any more!
Json::Value& null = null_();
Json::Value& nullRef = null_();

and replaced all the references to null in the JSONCPP code to null_(). This works now and I can use Json::Value in a constructor of a global object without it crashing.

[BillyGoto]

On Wednesday, June 22, 2016, Mark Lakata notifications@github.com wrote:

The Json::Value::null object is a global reference object. The rules of
C++ allow other constructors from other translation units to access the
Json::Value::null symbol before it is constructed, which means that the
reference is bogus, and you get a segmentation violation for accessing it.
That means if you have a global object that uses Json::Value in its
constructor, it can easily crash, depending on the whim of the linker
deciding what to link first.

The solution (which I have shown works in my private workspace) is to
replace Json::Value::null with static function, which has an 8 byte
static local variable inside. Then accesses to null() will always be
valid and not depend on a race condition of the global list.

const Value& Value::null()
{
static const unsigned char ALIGNAS(8) kNull[sizeof(Value)] = {0};
const unsigned char& kNullRef = kNull[0];
return reinterpret_cast<const Value&>(kNullRef);
}

and replaced all the references to null to null(). This works now and I
can use Json::Value in a constructor of a global object without it crashing.

Not a fan of this. It's undefined behavior, and relies on Value layout and
alignment details.

If you have access to "magic statics" (msft term) you can just new a Value
and return a ref to it.

const Value& Value::nullref() {
static auto p = new Value;
return *p;
}

Or something similar.

Question is whether to support the legacy null. If so, I propose renaming
this function to nullref() and then constructing const Value& Value::null
= nullref();

It was a mistake. Adding new ways to access it would double down on that
mistake, IMO.
I'd suggest that user code just stop using it altogether, citing the
init-time danger.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#488, or mute the
thread
https://github.com/notifications/unsubscribe/AHFzPElG2Df7hMQQucQ3H2UNozDHktUVks5qOfUYgaJpZM4I8aHd
.

ǝnɥɐuop ʎllıq

[marklakata]

I updated my proposal with a better implementation.

I think the existing code with the kNull et al was someone's failed attempt to fix the c++ construction fiasco - that wasn't my code, but I cut and pasted it into the Meyers Singleton. I've fixed that to be sane.

[cdunn2001]

The ALIGNAS crap came from the Chromium team (28836b8), in an effort to avoid a crash on ARM.

But that change had to be reverted (93f45d0) because it broke binary-compatibility.

I would never have included a global without a way to initialize, but that's a long time ago. Note that initialization is safe in an init function of a shared library in Linux, and there is a similar mechanism in Windows. That's a possibility, but not today.

Although we absolutely cannot remove these symbols, we can use a wrapper in our code, as @marklakata suggests. That should work for the vast majority of cases, and as @BillyGoto points out, nobody should be using it anyway.

Hi, Mark.

We currently use Json::Value::null directly in precisely one location, in src/test_lib_json/main.cpp, which is only a test:

 303   JSONTEST_ASSERT_EQUAL(Json::Value::null, null_);

We use nullRef is several places:

• https://github.com/open-source-parsers/jsoncpp/search?utf8=%E2%9C%93&q=nullRef

I don't think any of those are used by ctors, but that's not really the point. We don't need to refer to it directly.

I'll submit a PR with your code...