`json+struct` codec should check for a length problem with `!=` not `<`

[bhaller]

Making a new issue for this at the request of @jeromekelleher since #3306 was merged and closed. Tagging @petrelharp since he'll be a client of this API too, in pyslim.

Copy-paste from #3306:

Right now you have:

if ((uint64_t) metadata_length < total_length) {
    ret = tsk_trace_error(TSK_ERR_FILE_FORMAT);
    goto out;
}

Can I suggest that the test should be != rather than <? Seems like it ought to match exactly if the metadata is well-formed; and I had a subtle bug just now that had me tearing my hair out that would've been caught with a != test here. Is there a reason that you want to allow a mismatch here?

In followup discussion, @jeromekelleher wrote:

I think there's a good reason to be lenient here in allowing the length of the input buffer to be greater than the sum of the two segments - you might want to add padding for alignment. So, I think the current version is good.

I replied:

Hmm – this doesn't seem useful for alignment, since the length difference goes at the end...? For alignment, you want to have extra bytes inserted in between the JSON and the binary, which is not presently supported. (I suggested adding alignment bytes above, in fact; that would be quite a useful thing to provide in this PR!) As far as I can see, all that < does for the client is introduce the possibility of bugs. (I'm handling alignment myself in SLiM now, in fact, and the use of < here was in no way helpful for doing that; all it did was hide a bug from me.)

To expand on that a bit: any bytes added for alignment purposes have to be part of the binary blob itself, in the current design. There's no way for alignment bytes to be inserted in between the JSON and the binary, except for the client to put those bytes at the beginning of the binary blob themselves – in which case the two lengths should still be equal. It seems to me that it is never useful or correct for metadata_length > total_length to be true in the code above, so the correct test is with !=, not <. I have, in fact, implemented alignment bytes in SLiM, and the two lengths are still equal. But perhaps I'm missing the point somewhere?

[jeromekelleher]

I was thinking about alignment in the callers buffer rather than as stored, so the caller wouldn't necessarily have to have a buffer exactly the same size as the packed data. But it's a minor thing, I don't feel strongly about it either way.

[bhaller]

I was thinking about alignment in the callers buffer rather than as stored, so the caller wouldn't necessarily have to have a buffer exactly the same size as the packed data. But it's a minor thing, I don't feel strongly about it either way.

In SLiM, when reading in a .trees file, I'm doing:

int ret = tsk_json_struct_metadata_get_blob(p_tables.metadata, p_tables.metadata_length, &top_level_json_buffer, &top_level_json_length, &top_level_binary_buffer, &top_level_binary_length);

So I'm passing in the metadata length that tskit itself has for the metadata, which ought to be correct. That's presumably the 99% case, and for the other 1%, it seems like it would be very weird to pass in an incorrect length and have tskit just ignore whatever extra bytes come after the end of the binary blob. Even if the caller has their own metadata buffer for some reason, it seems like they ought to pass in the correct length for the actual data, not the (perhaps larger) allocated size of whatever buffer they're using. Agreed that it is a minor thing in some sense; but since the use of < already caused me to spend a fair bit of time hunting down a bug that = would have caught for me right off the bat, I do feel a bit strongly about it nevertheless.

In any case, I'd suggest making the check strict to begin with; if it bothers someone it can be relaxed later, whereas the opposite would not be possible since it would break code that used to run without errors.

[jeromekelleher]

Let's see how the discussion on alignment works out in #3423 and revisit once that's settled - but I'm assuming you're correct here and we'll switch to ==.