[1.x] don't keep null clones

[robUx4]

This will crash in the destructor and many other places.

It's unlikely to happen but better safe than sorry.

(cherry picked from commit ca27cb0) (edited)

[mbunkus]

Personally I'm not a fan of trying to handle out-of-memory in the library in this way. Most applications including MKVToolNix cannot handle out-of-memory well if at all as doing so requires quite a number of different-to-master techniques. For example, even the process of formatting & outputting useful error messages likely requires allocating even more memory, which will usually fail then, too. Most apps simply abort. I'd rather our libraries aborted in that case as well. It's a situation no sane application will ever be able to recover from.

[robUx4]

I agree when you're short on memory it's better to do as little as possible and return. Any program will likely crash if there's too little memory left.

However in this case we're talking about a clone. In the case of an EbmlBinary (or many of them) That could result in huge memory allocation (like large attachments in Matroska) even though there's still plenty of memory for small things.

In this case Clone() actually won't return nullptr so this won't be covered here, yet. But IMO the Clone() should return nullptr in that case. And thus we should be (and can be) ready for that case.

[mbunkus]

Even if the element's Clone could return nullptr, how you're trying to handle this in EbmlMaster isn't good. At the moment you're simply skipping elements for which this happens, silently hiding the problem. The application will never know that there was a problem; it cannot verify easily if the operation succeeded or failed.

Semantically this is a bit like a database transaction: if a database clients starts a transaction it can be sure at the end of it that either all of the actions taken within the transaction have been applied, or none of it. Back to libEBML, the result of EbmlMaster::Clone() should either be a new EbmlMaster with all children cloned successfully, or no new element at all. The mechanism you propose is pretty much the worst solution overall.

[robUx4]

We can change the API in 2.0 but this is a fix for 1.x. We can't throw in the EbmlMaster copy constructor. But we also can't push null in the vector because it will be used in the destructor (and other places). So this is avoiding some crashes.

However it will be in an unstable state. But I think overall it's better than a null dereferencing.

[mbunkus]

I kind of disagree that this is in any way better. I'd rather the application die immediately via std::abort(), maybe trying to output a corresponding message to std::cerr first. Letting a program run with silent data corruption is the worst thing to do IMO.

[robUx4]

OK, so should I revert #162 ?

[mbunkus]

Probably, yeah. Sorry, I only thought about this more after approving 162.

[robUx4]

No worries.