Fix ZSTD_readSkippableFrame with NULL dst

[akrieger]

Summary:
ZSTD_readSkippableFrame supports a NULL dst in the business logic but asserts if the frame size is greater than dst capacity. If dst is NULL, then dst capacity should be irrelevant.

Test Plan:
Read a skippable frame with a NULL dst just to get the frame size.

[Cyan4973]

If the goal is to read the size of the skippable frame content,
the method ZSTD_getFrameHeader() should provide this information.

Another relevant method would be ZSTD_findFrameCompressedSize(), though this one will return the size of the entire frame, thus including its headers (+ 8 bytes).

In contrast, ZSTD_readSkippableFrame()'s contract is to extract the content within a skippable frame, and write it into a provided dst buffer. If this content is not empty, it is expected for this function to exit with an error code if the provided destination buffer (NULL, 0) is not large enough.

[akrieger]

Aha, got it. Yeah ZSTD_getFrameHeader does give me what I want, and ZSTD_findFrameCompressedSize gets what I want in other cases as well.

I think I was mostly confused why the inconsistency in the assertion vs the defensive coding of the function itself.

Thanks!

[Cyan4973]

There may be some doc worth updating to reduce confusion

[akrieger]

It would be nice (although I suppose, not frequently requested) to have an api to retrieve a skippable frame's magic variant without being forced to read the entire frame also. This PR would allow that.

[Cyan4973]

It would be nice (although I suppose, not frequently requested) to have an api to retrieve a skippable frame's magic variant without being forced to read the entire frame also. This PR would allow that.

That's a good point.

The way I see it, ZSTD_getFrameHeader() would be a good place to provide this information.

The structure ZSTD_frameHeader contains many fields, most of which are inapplicable to Skippable frames, and therefore useless. One possibility here would be to piggy-back on one of the unused fields, and give it a different meaning in the context of Skippable frames. For example, dictID could be used to effectively mean magicVariant in the context of a Skippable frame.

If we were using C11, we could employ a nameless union to give 2 names for the same field, but this code base must remain C90 compatible, and nameless unions are not part of this spec.

If it feels too hackish, an alternative would be to employ one of the 2 remaining reserved fields for this purpose, and give it an explicit name (like skippableMagicVariant). A downside is that the structure would only have 1 reserved field left, and one the reasons these fields are there is to never have to change the size of this structure in the future, to preserve ABI compatibility.

[Cyan4973]

There is a PoC at #4228