[C++] Possible memory stomping in Object API

[dbaileychess]

In #6725 some generated code was added to unpack data directly into an existing pointer if it existed. However, this seems to allow it to stomp on the memory of the pointer if someone else is holding reference to it. It also seemed to cause a memory leak.

Need to investigate it more.

[dbaileychess]

I think the root cause is that the UnpackTo functions don't seem correct.

Here is a test I wrote that passes, but I think the last line is in error.

void UnPackTo(const uint8_t *flatbuf, size_t length) {
  // Get a monster that has a name and no enemy
  auto monster = GetMonster(flatbuf);
  TEST_EQ_STR(monster->name()->c_str(), "MyMonster");
  TEST_ASSERT(monster->enemy() == nullptr);

  // Create an enemy
  MonsterT* enemy = new MonsterT();
  enemy->name = "Enemy";

  // And create another monster owning the enemy,
  MonsterT mon1;
  mon1.name = "I'm monster 1";
  mon1.enemy.reset(enemy);

  // Assert that all the Monster objects are correct.
  TEST_EQ_STR(mon1.name.c_str(), "I'm monster 1");
  TEST_EQ_STR(enemy->name.c_str(), "Enemy");
  TEST_EQ_STR(mon1.enemy->name.c_str(), "Enemy");

  // Now unpack monster ("MyMonster") into monster 1
  monster->UnPackTo(&mon1);

  // Monster1 name should be from monster
  TEST_EQ_STR(mon1.name.c_str(), "MyMonster");

  // Enemy should still have its own name.
  TEST_EQ_STR(enemy->name.c_str(), "Enemy");

  // The monster1 shouldn't have any enemies, because monster didn't!
  TEST_EQ_STR(mon1.enemy->name.c_str(), "Enemy");
}

@aardappel Should unpacking a Monster into an existing MonsterT overwrite every field?

[dbaileychess]

@RWauson FYI

[aardappel]

Yes it would seem logical that if you're going to reuse an object then it should also reset fields not written.

[RWauson]

I'm not sure I agree. Since 'enemy' is a an optional field, my interpretation is that this message doesn't have any information about the state of the 'enemy' field. Since it doesn't have any info about it, then it shouldn't reset it.

This allows you to have a struct that keeps the total received state of that object.

In our use case, we have several sources that update a single entity, but each only updates a handful of fields.

e.g. One has knowledge of the 'enemy' state, but no knowledge of the HP.

As is, we can UnpackTo() our state variable and it updates every field that the message contains information about, without overriding fields that it doesn't.

If this behavior changes, there's no good alternative to replicate it

[dbaileychess]

I feel like it is really odd that you would end up with a mix state from multiple sources, as the default. The case you describe makes sense from a business logic point of view, but I don't think it should be the default for the library.

If anything, we should add a parameter to dictate if the receiving object is "reset" or not when it gets unpacked too. We can default it to false, and for your case, you can switch it to true.

[RWauson]

(I'm happy enough with a parameter to dictate that behavior, and it's fine for it not be the default)

It does seem more intuitive to me, from the nature of UnpackTo() (since you already have an object), that it would retain as much of the previous state as possible. Otherwise, the only benefit to UnpackTo() over Unpack() is optimization. Which is fine, but if I was coming at the library without any other prior knowledge, I would assume that the fields were retained.

[dbaileychess]

I think it's because UnPack always recreated a fresh state to put things in, that I would associate UnPackTo to do the same thing, just this time you specify where it occurs, and not create new memory.

I think I would have named it MergeInto or something, to indicate that the states might mix.

Thanks for your feedback though, it is good to hear how users expect things to work.

[aardappel]

Would agree that, by default, potentially getting state from an unrelated object would be bad.

There are 2 separate cases here: reusing because you want to save on memory allocation, and reusing because you want to merge semantically. The former is what is intended here, and what makes sense for a performance oriented library like this. The latter seems pretty niche to me.