Repository navigation
Crash in dump() from a static object #359
Description
Change 29c5f32 from the performance regression bug #272 introduces a potential crash in dump(). This has to do with deinitialization order of static objects.
The following code (not json specific) reproduces the problem (tested on GNU/Linux 64 bits with GCC 5.4):
#include <locale>
#include <sstream>
#include <iostream>
namespace {
struct DecimalSeparator : std::numpunct<char> {
char do_decimal_point() const { return '.'; }
};
}
struct test {
void dump() {
std::stringstream ss;
const static std::locale loc(std::locale(), new DecimalSeparator);
ss.imbue(loc);
ss << "test " << 1.0;
std::cout << ss.str() << std::endl;
}
~test() {
dump();
}
};
static test t;
int main() {
t.dump();
return 0;
}What happens here it that the static test is initialized first, then the first call to dump() creates the static locale object. When we leave main(), the static locale gets destroyed first (reversed initialization order), then the static test object is destructed, thus calling the destructor, which so happens to call dump() whose static object is no longer valid.
Removing the static keyword introduced in commit 29c5f32 fixes the crash.
Activity
nlohmann commentedon Nov 16, 2016
@gregmarr Do you have an idea how to fix this without getting back to the worse performance?
gregmarr commentedon Nov 16, 2016
The static is the entire reason for that commit. Removing it also returns the poor performance.
One fix is to not use the json library in the destructors of static objects, due to the indeterminate ordering.
If we don't want to impose that restriction, the only other way is to dynamically allocate that object and not free it. That may or may not resolve the issue, as json may depend on other things which have already been destroyed.
palacaze commentedon Nov 16, 2016
Yes it may be a reasonable restriction. I think I can change my code to avoid calling dump in the destructor, but this assumption needs to be documented.
gregmarr commentedon Nov 16, 2016
Avoiding code that relies on a particular order of global destruction is a good philosophy in general.
It's only dump(int) that has that direct restriction, but hash::operator() calls it, and so does an error handling path in patch(). There are two other static objects in the library. One is a constexpr std::array of a primitive type, and the other is a C array of a primitive type, so there are no destructors.
gregmarr commentedon Nov 16, 2016
Avoiding code that relies on a particular order of global destruction is a good philosophy in general.
It's only dump(int) that has that direct restriction, but hash::operator() calls it, and so does an error handling path in patch(). There are two other static objects in the library. One is a constexpr std::array of a primitive type, and the other is a C array of a primitive type, so there are no destructors.
gregmarr commentedon Nov 16, 2016
I haven't found anything else in here that has any dependencies on other global objects as long as the C++ standard library hasn't been terminate.
I did find another place where we might want to do the same thing as we did in dump(), around line 5900, and if we do decide to not free it, do the same here:
nlohmann commentedon Nov 16, 2016
Would it help change the
locobject as static member of the class?gregmarr commentedon Nov 16, 2016
I don't think that will change when it is destroyed. The problem is that it is destroyed at all. By having a static object, the destructor has to run for it at some point. Once that happens, any calls to the library from the destructor of another static object will access uninitialized memory. The way to avoid that is to make it a static pointer instead, so that the destructor never runs.
gregmarr commentedon Nov 16, 2016
Example, untested:
palacaze commentedon Nov 17, 2016
I can confirm the proposed alternative approach to the static keyword fixes the crash for me.
nlohmann commentedon Nov 17, 2016
But wouldn't this mean leaking memory which would also be worth noting in the README?
gregmarr commentedon Nov 17, 2016
Yes, it would be memory that would not be freed on exit, and it should be noted in the README as well as in a comment above it.
palacaze commentedon Nov 17, 2016
If you care about minimizing those, you may want to share the same static instance, through a static private function like in the basic_json class like so:
and use it in both bump() and operator<<().
gregmarr commentedon Nov 17, 2016
or even move it out of the class and into the namespace.
TurpentineDistillery commentedon Nov 21, 2016
Maybe instead of looking for the correct way to solve the problem just avoid it altogether by yanking the whole DecimalSeparator business and imbue with std::locale::classic() ?
gregmarr commentedon Nov 21, 2016
@TurpentineDistillery Oooh, much better. I had never run into that one before.
🐛 fixing #359
nlohmann commentedon Nov 21, 2016
Thanks @TurpentineDistillery for hinting to
std::locale::classic()!With 43dbe02 I changed the code accordingly. The performance of
dumpremains the same:@palacaze Does this fix your problem?
palacaze commentedon Nov 21, 2016
I can't reproduce the crash with this change, so as far as I am concerned the bug has been fixed.
nlohmann commentedon Nov 21, 2016
Thanks for the quick feedback! I shall merge the feature branch once the Travis builds have completed.
Thanks everyone!
🐛 fixes crash in dump from static object (#359)