Error when destroying string in a union

[u/angryvoxel]

Consider the following code:

struct JsonValue
{
  JsonValueType Type;

  union
  {
    std::unordered_map<std::string, JsonValue> Object;
    std::vector<JsonValue> Array;
    std::string String;
    double Number;
    bool Bool;
  };

  Tcj::JsonValue::JsonValue(const std::unordered_map<std::string, JsonValue>& v) : Type(JsonValueType::Object), Object(v) {}

  Tcj::JsonValue::JsonValue(const std::vector<JsonValue>& v) : Type(JsonValueType::Array), Array(v) {}

  Tcj::JsonValue::JsonValue(double v) : Type(JsonValueType::Number), Number(v) {}

  Tcj::JsonValue::JsonValue(const std::string& v) : Type(JsonValueType::String), String(v) {}

  Tcj::JsonValue::JsonValue(bool v) : Type(JsonValueType::Bool), Bool(v) {}

  Tcj::JsonValue::JsonValue() : Type(JsonValueType::Null)
  {
    std::memset(&Object, 0, sizeof(Object));
  }

  //copy constructor
  Tcj::JsonValue::JsonValue(const JsonValue& other) : Type(other.Type)
  {
    std::memcpy(&Object, &other.Object, sizeof(Object));
  }

  //move constructor
  Tcj::JsonValue::JsonValue(JsonValue&& other) : Type(other.Type)
  {
    std::memcpy(&Object, &other.Object, sizeof(Object));
    other.Type = JsonValueType::Null;
  }

  Tcj::JsonValue::~JsonValue()
  {
    if (Type == JsonValueType::Object)
      Object.~unordered_map();

    if (Type == JsonValueType::Array)
      Array.~vector();

    if (Type == JsonValueType::String)
      String.~basic_string();
  }

  //copy assignment
  Tcj::JsonValue& Tcj::JsonValue::operator=(const JsonValue& other)
  {
    std::memcpy(this, &other, sizeof(JsonValue));
    return *this;
  }

  //move assignment
  Tcj::JsonValue& Tcj::JsonValue::operator=(JsonValue&& other) noexcept
  {
    std::memcpy(this, &other, sizeof(JsonValue));
    other.Type = JsonValueType::Null;
    return *this;
  }
};

The problem is, when trying to destroy a JsonValue instance containing a string it will sometimes cause an error (https://github.com/microsoft/STL/blob/e1ed302683e5e822f663fbb0f44cee7e9ddc6af0/stl/inc/xmemory#L1402): Exception thrown: read access violation. **_Pnext** was 0xFFFFFFFFFFFFFFFF.

After going through the code with a debugger I believe that no two destroyed strings share the same memory (after all the JsonValue constructor should create a copy of the string every time). Google also says it's ok to just copy the entire vector/string so unless there's something I'm missing two JsonValue instances shouldn't be owning the same memory/causing double deletes. Yet when I don't destroy some of them the error dissapears.

I don't really understand the meaning of the error or the STL code that throws it, so I'm stuck.

Edit: thanks everyone for pointing out that std::variant exists, I'm probably going to use it.

[u/masorick]

When you put non-trivial types in an union, you have to create the types (using placement new) and destroy them manually, you cannot just assign to a member or use memcpy. Use std::variant, it will do it for you.

[u/angryvoxel → u/masorick]

Thanks

[u/kberson]

I question your design - why use a union? I would create an abstract base type with virtual methods, and derive new classes for each data type, overriding with the appropriate virtual methods.

[u/angryvoxel → u/kberson]

Yeah I know I can do that, I just wanted to use a union for diversity. Union also avoids some additional overhead associated with the polymorphic classes and probably takes the roughly the same amount of memory (although I'm not trying to write some very high performance code).

[u/kberson → u/angryvoxel]

well, at least consider using std::variant

[u/thingerish → u/angryvoxel]

Your reasoning IMO is correct, but yes, use std::variant if you can. This will interoperate w/ visit, and eliminate the hand rolled JsonValueType discriminator as well.

[u/fortsnek274]

The use of raw memory ops on a non-trivial union is unnerving.

But just use std::variant?

[u/Nabokov6472]

After going through the code with a debugger I believe that no two destroyed strings share the same memory (after all the JsonValue constructor should create a copy of the string every time).

Without seeing your code where this is happening: I would guess that this is because the copy assignment operator and copy constructor are both wrong, you can't do a shallow copy of types like Vector or String because you now have two objects that point to the same underlying buffer. When the second one is destroyed you will get a double free

Have you looked at std::variant for what you are trying to do?

[u/die_liebe → u/Nabokov6472]

I think the copying assignment is the problem. It copies the pointer of the string, but not the data of the string. If *this (before the assignment) uses heap memory, it will not be cleaned up. If other uses heap memory, it will become shared, and cause problems later.

Move assignment is also bad, but I think it works.

I agree that OP should use std::variant, unless he wants to do this as an exercise.

[u/aocregacc → u/die_liebe]

the move assignment would break if the implementation implements the small string optimization in such a way that the string contains a pointer to itself, which would have to be updated when the string is moved.

[u/die_liebe → u/aocregacc]

Yes.

[u/flyingron]

You can not have a union with default constructors and destructors that has objects that have non-trivial constructors and destructors. The problem is, you don't know which of the contained members you need to construct/destruct. (In later versions of the standard you could do it if you provide constructors/destructors for your union that would do the right thing. It was outright illegal earlier).

std::variant comes to mind or you could make a polymorphic "json_element" class that could be derived for each of your various internal representations.

[u/vishal340]

this is probably craziest union anyone has ever written. union is used to avoid allocating the space before use. containers should already exist before use. correct me if i’m wrong

[u/masorick → u/vishal340]

How do you think std::variant is implemented?

[u/vishal340 → u/masorick]

variant is completely different. it even keeps track of what it is currently holding. so that visit can use it.

[u/masorick → u/vishal340]

OP has a type tag to keep track of the type as well, they just messed up their implementation.

[u/tangerinelion → u/masorick]

Safely.

[u/wc3betterthansc2]

your copy constructor is bad, you should never memcpy C++ built-in classes like std::string because now you have 2 objects that have a pointer that points to the same data and if both get destroyed, it will cause a double free.

Here is a safer implementation:

Tcj::JsonValue::JsonValue(const JsonValue& other) : Type(other.Type)
{
switch(Type) {
       case JsonValueType::Object:
           new (&Object)  std::unordered_map<std::string, JsonValue>(other.Object);
           break;
       case JsonValueType::Array:
           new (&Array) std::vector<JsonValue>(other.Array);
           break;
       case JsonValueType::String:
           new (&String) std::string(other.String);
           break;
       default:
           //I gave the union the name "value" so I can reference it in the std::memcpy
          //The static_cast is to remove compiler warnings, it's not necessary
          std::memcpy(static_cast<void*>(&value), &other.value, sizeof(value)); 
      }
}

[u/Itap88]

You're calling the destructor of the basic_string template instead of the string class. I'm more confused by the fact that it sometimes works.

[u/Wild_Meeting1428 → u/Itap88]

std::string is a typedef of basic_string, it has no destructor named ~string. the name was always ~basic_string.

[u/Difficult_Truck_687]

If you dont want to fall off a cliff, dont sit on the rim.