From 7616cfe8545969e2217fae3e16409895e2ac8ad4 Mon Sep 17 00:00:00 2001 From: Kovid Goyal Date: Sun, 23 Aug 2026 17:05:06 +0530 Subject: [PATCH] Graphics: Make reading images robust against clients that truncate files that are being read by kitty --- docs/changelog.rst | 8 ++++ kitty/graphics.c | 88 ++++++++++++++++++++++------------------- kitty/graphics.h | 3 -- kitty_tests/graphics.py | 35 ++++++++++++++++ 4 files changed, 91 insertions(+), 43 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 51b86ae13..dc5f10a4e 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -264,6 +264,14 @@ Detailed list of changes - Text sizing protocol: Fix a buffer overflow when a natural width (no explicit ``w`` key) text sizing escape code contains a grapheme cluster longer than four codepoints +- Graphics protocol: Fix a crash when transmitting image data via a file or + shared memory object (``t=f``, ``t=t`` or ``t=s``) and the client truncates + it while kitty is reading from it. + +- Graphics protocol: Fix reading image data from a file or shared memory + object at an offset (the ``O`` key) failing unless the offset happened to be + a multiple of the system page size + 0.48.2 [2026-07-30] ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/kitty/graphics.c b/kitty/graphics.c index 575029427..79a2307cf 100644 --- a/kitty/graphics.c +++ b/kitty/graphics.c @@ -111,9 +111,6 @@ free_load_data(LoadData *ld) { ld->buf_used = 0; ld->buf_capacity = 0; ld->buf = NULL; - if (ld->mapped_file) munmap(ld->mapped_file, ld->mapped_file_sz); - ld->mapped_file = NULL; - ld->mapped_file_sz = 0; ld->loading_for = (const ImageAndFrame){0}; } @@ -328,19 +325,42 @@ set_command_failed_response(const char *code, const char *fmt, ...) { } static bool -mmap_img_file(GraphicsManager *self, int fd, size_t sz, off_t offset) { - if (!sz) { - struct stat s; - if (fstat(fd, &s) != 0) ABRT(EBADF, "Failed to fstat() the fd: %d file with error: [%d] %s", fd, errno, strerror(errno)); - sz = s.st_size; +read_img_file(GraphicsManager *self, int fd, size_t sz, off_t offset, size_t max_to_read) { + LoadData *ld = &self->currently_loading; + struct stat s; + if (fstat(fd, &s) != 0) ABRT(EBADF, "Failed to fstat() the fd: %d file with error: [%d] %s", fd, errno, strerror(errno)); + // The graphics protocol specification mandates that only regular files be + // read. Reading from FIFOs/devices/etc. can block forever or have + // side-effects. + if (!S_ISREG(s.st_mode)) ABRT(EBADF, "The image file with fd: %d is not a regular file", fd); + if (!sz) sz = offset < s.st_size ? (size_t)(s.st_size - offset) : 0; + if (sz > max_to_read) sz = max_to_read; + free(ld->buf); + // Note that the data is copied rather than mmap()ed as the client is free + // to truncate the file at any time, which would cause SIGBUS when reading + // from a mapping of it. With read() a truncated file simply results in a + // short read, which is reported as insufficient data by the caller. + ld->buf = malloc(sz ? sz : 1); + if (!ld->buf) ABRT(ENOMEM, "Out of memory allocating %zu bytes to read image file", sz); + ld->buf_capacity = sz; + ld->buf_used = 0; + while (ld->buf_used < sz) { + ssize_t n = pread(fd, ld->buf + ld->buf_used, sz - ld->buf_used, offset + (off_t)ld->buf_used); + if (n < 0) { + if (errno == EINTR) continue; + ABRT(EIO, "Failed to read from image file fd: %d with error: [%d] %s", fd, errno, strerror(errno)); + } + if (!n) break; // EOF, the file is smaller than expected, reported as insufficient data by the caller + ld->buf_used += n; } - void *addr = mmap(0, sz, PROT_READ, MAP_SHARED, fd, offset); - if (addr == MAP_FAILED) - ABRT(EBADF, "Failed to map image file fd: %d at offset: %zd with size: %zu with error: [%d] %s", fd, offset, sz, errno, strerror(errno)); - self->currently_loading.mapped_file = addr; - self->currently_loading.mapped_file_sz = sz; return true; err: + // Discard any partially read data, the caller is not guaranteed to call + // free_load_data() on failure. + free(ld->buf); + ld->buf = NULL; + ld->buf_capacity = 0; + ld->buf_used = 0; return false; } @@ -635,7 +655,11 @@ load_image_data( ABRT("EPERM", "Permission denied to read image file"); } } - load_data->loading_completed_successfully = mmap_img_file(self, fd, g->data_sz, g->data_offset); + // When the data needs further processing the entire (possibly + // compressed) payload is needed, otherwise reading more than the + // expected number of bytes is pointless. + const size_t max_to_read = (g->compressed || data_fmt == PNG) ? MAX_DATA_SZ : load_data->data_sz; + load_data->loading_completed_successfully = read_img_file(self, fd, g->data_sz, g->data_offset, max_to_read); safe_close(fd, __FILE__, __LINE__); if (transmission_type == 't' && strstr(fname, "tty-graphics-protocol") != NULL) { if (global_state.boss) { @@ -650,20 +674,15 @@ load_image_data( } static Image * -process_image_data(GraphicsManager *self, Image *img, const GraphicsCommand *g, const unsigned char transmission_type, const uint32_t data_fmt) { +process_image_data(GraphicsManager *self, Image *img, const GraphicsCommand *g, const uint32_t data_fmt) { bool needs_processing = g->compressed || data_fmt == PNG; if (needs_processing) { uint8_t *buf; size_t bufsz; -#define IB \ - { \ - if (self->currently_loading.buf) { \ - buf = self->currently_loading.buf; \ - bufsz = self->currently_loading.buf_used; \ - } else { \ - buf = self->currently_loading.mapped_file; \ - bufsz = self->currently_loading.mapped_file_sz; \ - } \ +#define IB \ + { \ + buf = self->currently_loading.buf; \ + bufsz = self->currently_loading.buf_used; \ } switch (g->compressed) { case 'z': @@ -691,21 +710,10 @@ process_image_data(GraphicsManager *self, Image *img, const GraphicsCommand *g, if (self->currently_loading.buf_used < self->currently_loading.data_sz) { ABRT("ENODATA", "Insufficient image data: %zu < %zu", self->currently_loading.buf_used, self->currently_loading.data_sz); } - if (self->currently_loading.mapped_file) { - munmap(self->currently_loading.mapped_file, self->currently_loading.mapped_file_sz); - self->currently_loading.mapped_file = NULL; - self->currently_loading.mapped_file_sz = 0; - } } else { - if (transmission_type == 'd') { - if (self->currently_loading.buf_used < self->currently_loading.data_sz) { - ABRT("ENODATA", "Insufficient image data: %zu < %zu", self->currently_loading.buf_used, self->currently_loading.data_sz); - } else self->currently_loading.data = self->currently_loading.buf; - } else { - if (self->currently_loading.mapped_file_sz < self->currently_loading.data_sz) { - ABRT("ENODATA", "Insufficient image data: %zu < %zu", self->currently_loading.mapped_file_sz, self->currently_loading.data_sz); - } else self->currently_loading.data = self->currently_loading.mapped_file; - } + if (self->currently_loading.buf_used < self->currently_loading.data_sz) { + ABRT("ENODATA", "Insufficient image data: %zu < %zu", self->currently_loading.buf_used, self->currently_loading.data_sz); + } else self->currently_loading.data = self->currently_loading.buf; self->currently_loading.loading_completed_successfully = true; } return img; @@ -819,7 +827,7 @@ handle_add_command(GraphicsManager *self, const GraphicsCommand *g, const uint8_ img = load_image_data(self, img, g, tt, fmt, payload); if (!img || !self->currently_loading.loading_completed_successfully) return NULL; self->currently_loading.loading_for = (const ImageAndFrame){0}; - img = process_image_data(self, img, g, tt, fmt); + img = process_image_data(self, img, g, fmt); if (!img) return NULL; size_t required_sz = (size_t)(self->currently_loading.is_opaque ? 3 : 4) * self->currently_loading.width * self->currently_loading.height; if (self->currently_loading.data_sz != required_sz) @@ -1752,7 +1760,7 @@ handle_animation_frame_load_command(GraphicsManager *self, GraphicsCommand *g, I img = load_image_data(self, img, g, tt, fmt, payload); if (!img || !load_data->loading_completed_successfully) return NULL; self->currently_loading.loading_for = (const ImageAndFrame){0}; - img = process_image_data(self, img, g, tt, fmt); + img = process_image_data(self, img, g, fmt); if (!img || !load_data->loading_completed_successfully) return img; const unsigned long bytes_per_pixel = load_data->is_opaque ? 3 : 4; diff --git a/kitty/graphics.h b/kitty/graphics.h index 90b08892e..f120e5315 100644 --- a/kitty/graphics.h +++ b/kitty/graphics.h @@ -135,9 +135,6 @@ typedef struct { uint8_t *buf; size_t buf_capacity, buf_used; - uint8_t *mapped_file; - size_t mapped_file_sz; - size_t data_sz; uint8_t *data; bool is_4byte_aligned; diff --git a/kitty_tests/graphics.py b/kitty_tests/graphics.py index d58d53b0f..d77f3d0aa 100644 --- a/kitty_tests/graphics.py +++ b/kitty_tests/graphics.py @@ -463,6 +463,41 @@ class TestGraphics(BaseTest): s.reset() self.assertEqual(g.disk_cache.total_size, 0) + def test_load_images_from_file_edge_cases(self): + s, g, pl, sl = load_helpers(self) + random_data = byte_block(32 * 1024) + + with tempfile.NamedTemporaryFile(prefix='tty-graphics-protocol-') as f: + # A window of the file specified with a non page aligned offset + f.write(b'x' * 3 + random_data + b'y' * 5), f.flush() + sl(f.name, s=1024, v=8, t='f', S=len(random_data), O=3, expecting_data=random_data) + + # A file that is truncated after the size declared in the command + # must be reported as insufficient data rather than crashing + f.seek(0), f.truncate(), f.write(random_data[:128]), f.flush() + self.ae(pl(f.name, s=1024, v=8, t='f', S=len(random_data)), f'ENODATA:Insufficient image data: 128 < {len(random_data)}') + + # Ditto when the size is not declared and is read from the file itself + self.ae(pl(f.name, s=1024, v=8, t='f'), f'ENODATA:Insufficient image data: 128 < {len(random_data)}') + + # An offset past the end of the file + self.ae(pl(f.name, s=1024, v=8, t='f', O=4096), f'ENODATA:Insufficient image data: 0 < {len(random_data)}') + + # Only regular files may be read + with tempfile.TemporaryDirectory(prefix='tty-graphics-protocol-') as tdir: + fifo = os.path.join(tdir, 'fifo') + os.mkfifo(fifo) + self.assertTrue(pl(fifo, s=1024, v=8, t='f').startswith('EBADF:'), 'Reading from a FIFO was not refused') + + # A shared memory object truncated to less than the declared size + name = '/kitty-test-shm-truncated' + shm_write(name, random_data[:64]) + self.ae(pl(name, s=1024, v=8, t='s', S=len(random_data)), f'ENODATA:Insufficient image data: 64 < {len(random_data)}') + self.assertRaises(FileNotFoundError, shm_unlink, name) # check that the object was deleted + + s.reset() + self.assertEqual(g.disk_cache.total_size, 0) + @unittest.skipIf(Image is None, 'PIL not available, skipping PNG tests') def test_load_png(self): s, g, pl, sl = load_helpers(self)