Investigate LZF Compressor memory access issues

[james7132]

As mentioned in #5, there are memory access issues with the LZFCompressor implementation that are causing Unity to crash. This should not be happening on any input.

With #5's merge, it will be disabled by default, but it is key in reducing bandwidth usage and ensuring messages are below max message size limits.

This should be investigated and reenabled once this has been resolved.

[toipi]

Hey! I'm pretty sure the crash in question happens when you try to decompress invalid data (e.g. uncompressed data as input, or input that got corrupted during transport despite having a valid checksum). This causes a StackOverflowException to be thrown and crashes Unity (possibly because of the unsafe context?) since the Decompress function stackallocs more memory upon failure and tries again until it succeeds.

The solution (on my end at least) was to add a sizeLimit parameter as an upper bound for the size of the decompression buffer. A reasonable value could be some constant depending on the use case, or a multiple of inputLength as an estimate for the highest possible decompressed size, but this kind of thinking might not cover all cases. The ideal value would be the available stack size since stackalloc is obviously stack-bound, but I couldn't find any resources on how that could be done. I'm also not sure if such an approach would play nice with async code. The C# reference page for stackalloc recommends setting a conservative size limit.

Some ideas for workarounds:

• In the case of networking code for games, it wouldn't be unreasonable to assume that you're working with some sort of MTU (e.g. 1200 bytes) and that you only want to attempt compression once before sending (i.e. max 1200 bytes allowed pre-compression), but it's definitely not a one size fits all solution. (e.g. in the case of fragmentation, packet fragments would have to be compressed individually, which is not ideal)

• Another approach might simply be to foresee errors by trying to decompress any freshly compressed data. If there's a decompression error due to our estimated sizeLimit being too low, we send the data in its uncompressed form. Again, not at all ideal, but it would prevent the stack overflow.

I don't currently use this library but you can test this with the following change to the Decompress function:

    /// <summary>
    /// Decompress input bytes.
    /// </summary>
    /// <param name="input">Bytes to decompress.</param>
    /// <param name="outputBuffer">Output/work buffer. Upon completion, will contain the output.</param>
    /// <param name="inputLength">Length of data in inputBytes.</param>
    /// <param name="startOffset">The offset into the input buffer to start decompressing from.</param>
    /// <param name="sizeLimit">The upper bound for the uncompressed output length in bytes. </param>
    /// <returns>Length of output. -1 if decompression fails.</returns>
    public static unsafe int Decompress(byte[] input, ref byte[] output, int inputLength, uint startOffset = 0, uint sizeLimit = 0)
    {
        if (sizeLimit == 0) sizeLimit = uint.MaxValue; // should be set to a reasonable upper bound that won't cause a stack overflow

        // If outputSize is 0, increase buffer size and try again.
        int outputSize = input.Length;
        fixed (byte* inputPtr = input)
        {
            while (true)
            {
                byte* buffer = stackalloc byte[outputSize];
                int count = TryDecompress(inputPtr + startOffset, buffer, inputLength, outputSize);
                outputSize *= 2;

                // enforce size limit to avoid stack overflow
                if (outputSize > sizeLimit)
                {
                    return -1;
                }

                if (count == 0) continue;
                CopyBuffer(buffer, ref output, count);
                return count;
            }
        }
    }

I don't definitively know if this is the only thing that would cause such a crash of course, but in my case it seemed to be. If the crash goes away and you start getting -1 as the return value, you can then log the offending bits of data to see what the underlying error is.

I'd send a proper fix but I'm not sure how one could decide a reasonable upper bound outside my own use case. Hope this is of some help anyway. Thank you for your work on the library btw! :)