Repository navigation
libghostty-vt: let the png decode hook report extra allocated bytes - #14133
ArneshBanerjee wants to merge 1 commit into
Conversation
| // A cap smaller than the pixel data is a broken callback: the pixels | ||
| // it reported don't fit in the allocation it reported. Free what it | ||
| // said it allocated so we don't leak, then reject the image. | ||
| if (capacity < out.data_len) { | ||
| alloc.free(result_data[0..capacity]); | ||
| return error.InvalidData; | ||
| } |
There was a problem hiding this comment.
I'm curious about this. Wouldn't it be better to store the amount of additional memory that had been allocated? Then you can always just free result_data[0 . .out.data_len + out.data_extra])?
There was a problem hiding this comment.
That is better. Switched to data_extra.
It makes the bad state unrepresentable. With a total cap, a value below data_len is meaningless but still expressible, so the wrapper had to check for it and reject. With the surplus there is nothing to check, and 0 naturally means no extra.
I kept a checked add, since both lengths come from the callback and a wrapping sum would free a too-short slice. On overflow it rejects without freeing.
Force-pushed as it reworks the same field.
8555fd9 to
ba874e9
Compare
ba874e9 to
aa2a596
Compare
GhosttySysImage only carried data_len, and the library freed the pixel buffer with that length. The allocator needs the real allocation length to free, so a decode callback had to allocate exactly the pixel data. A buffer rounded up to a block size, or one reused across images, had no way to be described. Add data_extra, the number of bytes allocated beyond data_len. The library frees data_len + data_extra and still reads data_len bytes of pixels. Leaving it at 0 means the allocation is exactly data_len, so existing callbacks keep working unchanged. Reporting the surplus rather than the total means a length below the pixel data cannot be expressed, so there is no invalid combination to validate. The add is checked, since both lengths come from the callback. Fixes ghostty-org#12163
aa2a596 to
ae6f155
Compare
|
I rebased this on main because it had a merge conflict. Main now zeroes the decode output struct, so I kept that and removed my own zero-init. I also updated the header docs, which said "all four fields" and "exact size". Both are no longer true with |
jcollie
left a comment
jcollie
left a comment
There was a problem hiding this comment.
This looks good to me, but the PR description is out of date.
Updated it :) Should I update the PR title to "let the png decode hook report extra allocated bytes"? It seems more appropiate. |
Sure. Now we just need to wait for someone on the terminal team to approve. |
ArneshBanerjee
changed the title
Fixes #12163
GhosttySysImage only had data_len, and the library used that length to free
the pixel buffer. The allocator needs the real allocation length to free a
buffer. Thus a decode callback had to allocate exactly the size of the pixel
data. A callback could not report a buffer that it rounded up to a block size,
or a buffer that it reused across images.
This adds data_extra, the number of bytes that the callback allocated after
data_len. The library frees data_len + data_extra bytes and reads only data_len
bytes of pixels. The library sets the output struct to zero before it calls the
callback, so a callback that does not set data_extra gets 0. Existing callbacks
work without changes.
The field holds the surplus, not the total. Thus a callback cannot report an
allocation that is smaller than the pixel data, and there is no invalid value
to check. Both lengths come from the callback, so the library adds them with an
overflow check. If the sum overflows, the library rejects the image and does
not free the buffer.
On the Zig side, sys.Image has a new capacity field and a deinit that frees
with it. Thus the free is in one location and not at each call site. The header
docs and the kitty graphics example are updated to match.
Tested with zig build test-lib-vt. New tests in src/terminal/c/sys.zig cover a
callback with surplus, a callback with no surplus, and lengths that overflow.