Framebuffer overflow protection

[dmitrylyzo]

Quick fix for #120 (comment)

Pre-existing issue (so new commit to fix), but this line has potentially a big issue: Afaict there's no prior checks for this and the multiplications for the new_size parameter can overflow the bounds of int, which is in itself already undefined behaviour, but if your lucky only truncates. Worse though, the buffer is later accessed by values recalculated from width and height, so if the multiplication here overflowed (and the overflow itself doesn't blow up), an out-of-bound write happens either blowing up or corrupting some other memory. Is ReusableBuffer ever used for something else than an float-RGBA canvas alter down the line in jellyfin-patches? If not it's probably best to pass width, height and member_size as separate size_t-type values to the resize function and do all the overflow checking inside the function.

Personally, I don't like re-calculation of size at this line memset(buf, 0, sizeof(float) * width * height * 4);
Stored in a variable before calling buffer_resize.

I am thinking about:

typedef struct {
    float *buffer;
    size_t size;        // save the new_size
    size_t capacity;    // as the size member before
    size_t lessen_counter;
} buffer_t;

Storing the size is not suitable for ReusableBuffer. But it is fine for FrameBuffer.

[dmitrylyzo]

Changing buffer_resize in this way makes keep_content unnecessary, since it becomes a framebuffer (image) instead of a general-purpose buffer.

But, as I said before, a raw dynamic buffer may be needed in the future, e.g. to replace m_is_event_animated.
A template dynamic array would be better if it had no overhead.

So it might be better to refactor this in class first. Then wrap it with another FrameBuffer class, like:

class FrameBuffer {
private:
    ReusableBuffer m_buffer;

public:
    float* ensure_size(size_t width, size_t height) {
        if (width == 0 || width > FRAMEBUFFER_MAX_WIDTH || height == 0 || height > FRAMEBUFFER_MAX_HEIGHT) {
            return NULL;
        }

        const size_t new_size = width * height * 4 * sizeof(float);

        return (float*)m_buffer.ensure_size(new_size, false);
    }

    size_t size() const {
         return m_buffer.size();
    }
};

[dmitrylyzo]

Because of this it will be difficult to use the mentioned FrameBuffer class: need to know the size before use and unsigned int instead of float.

Would it be sufficient to check the size in the resizeCanvas?
How hard do we need to defend ourselves against libass? Assuming libass won't return out-of-range images.
The downside: when I tried 200x200 as a max, the subtitles wasn't scaled to the player size because the main thread doesn't know about the trimming.

[dmitrylyzo]

Hmm, what about <= 0? Exit with some code or set some default values?
Also, portrait orientation 🤔 Lazy option - FRAMEBUFFER_MAX_SIZE = 8192 for both.

[TheOneric]

Using custom magic constant to guard against type overflows is incorrect. Superseded by #133.