OPS savefiles (palette key) break BSON specification, preventing interoperability

[tigercat2000]

Hi,

In GameSave.cpp#2481-2486

bson_append_start_array(&b, "palette");
for(auto iter = paletteData.begin(), end = paletteData.end(); iter != end; ++iter)
{
    bson_append_int(&b, (*iter).first.c_str(), (*iter).second);
}
bson_append_finish_array(&b);

bson_append_start_array is used, and then bson_append_int is called with a key of (*iter).first.c_str().
bson_append_start_array uses the type BSON_ARRAY, which according to the BSON 1.1 specification must follow this format:

Array - The document for an array is a normal BSON document with integer values for the keys, starting with 0 and continuing sequentially. For example, the array ['red', 'blue'] would be encoded as the document {'0': 'red', '1': 'blue'}. The keys must be in ascending numerical order.

This means that the savefiles contain invalid BSON - this breaks other parsers, such as bson-rust - it just so happens to be accepted by the particular bson.cpp the powder toy uses and thus isn't noticed.

To fix this, bson_append_start_object should be called instead.

bson_append_start_object(&b, "palette");
for(auto iter = paletteData.begin(), end = paletteData.end(); iter != end; ++iter)
{
    bson_append_int(&b, (*iter).first.c_str(), (*iter).second);
}
bson_append_finish_object(&b);

and accordingly, loading should instead check for BSON_OBJECT.

[git4rker]

why didn't you open a pull request?

[tigercat2000]

why didn't you open a pull request?

I'm not sure how to version this correctly in this codebase, it's a breaking change

[LBPHacker]

This has been known for a long time and we've decided that because there's nothing we need to be interoperable with, it's a non-issue.

As a fix of the bureaucratic variety, we would accept a PR that renames BSON.cpp/h to PowderON.cpp/h.

[tigercat2000]

I mean I'm working on a simulator that takes in OPS savefiles in Rust, so that's one thing that it's a problem for lol

[LBPHacker]

Welcome to tech, where technical debt is cumulative and mistakes made 10 years ago affect you today :)

[LBPHacker]

To be clear, I'm being depressing about this for more than just the fun of it. I see no economic way to fix this. We do try to maintain compatibility with old clients at least to the point of having them be able to render a partial thumbnail of new saves, and fixing this problem would make that impossible. In light of the interoperability requirements we have that I explained above (none), this would not be economical.

[jacob1]

I manage to parse it in python, because the library I use doesn't care about all the keys I don't use. If you don't fetch palette info, are you able to parse the save data?

[simtr]

Changing this to use BSON_OBJECT should still be reasonably backwards compatible. Palette data is optional-ish as the game will fall back to the compiled element map and should still be able to render thumbnails most of the time.
But at the same time, the save format wasn't intended for interoperability, though the binary format of array and object are the same so patching \x04palette\x00 sequence to \x03palette\x00 should work if you need to parse using another library.