Spec cleanup: Should fixup behavior when repeat1-1==0 be specified or changed to an error?

[elasota]

Doing some more edge case checks on the specification...

In ZSTD_decodeSequence there is this code:

size_t temp = (offset==3) ? seqState->prevOffset[0] - 1 : seqState->prevOffset[offset];
temp += !temp;   /* 0 is not valid; input is corrupted; force offset to 1 */

In the case where literals_length==0 and offset_value==3 and Repeated_Offset1==1 this will insert 1 at the start of the repeated offset list and move the rest down.

Right now the comment says it is invalid, but it is silently processed as if it is valid. So, should this behavior be put in the spec? If not, should the code be rejecting this data?

[Cyan4973]

This is invalid.
If the code reaches that part, it means data is corrupted, because it's trying to generate an offset = 0.

This is considered a potential attack vector, that's why the offset is converted into a 1: it ensures that the underlying memory content of the destination buffer is overwritten, hence it prevents scenario where such content could be accessed afterwards.

Detecting this issue is good ground to generate an error.
Problem is, inserting an error detection signal at this place in the code results (or at least used to result) in substantial performance drawbacks, because it's an extremely hot section of the decoding loop.
Therefore, we rather rely on the checksum to tell us so (assuming the issue is accidental, following a transmission or storage error).

[terrelln]

This is one of the cases where we don't guarantee that we reject all possible invalid frames. Since we guarantee safety on invalid frames, we must not actually copy from offset 0. In this case we decide to copy from offset 1. So given this invalid frame, for performance reasons, we decide to ignore the corruption.

Other implementations are allowed to reject this frame.

[Cyan4973]

spec updated