From 3cd66d2fd7ee892c623d0d353b4319b3c4db895f Mon Sep 17 00:00:00 2001 From: NRK Date: Sun, 8 Mar 2026 01:11:09 +0000 Subject: [PATCH] command: avoid crashing the player with mmap mmap in inherently crashy due to the file changing underneath and requires esoteric SIGBUS handling [0] to use properly. to reproduce, simply use "overlay-add" from a lua script with invalid stride set and observe mpv crashing with SIGBUS. here we're just copying the file content (introduced in 549a9ea) and munmaping it immediately so there's no real need to be using mmap to begin with. avoid the issue entirely by just reading the file directly with fseek+fread with optimization for the common stride case. one notable difference is that since dup-ed fds share same offset, the source "fd" that's coming from client will now have it's offset modified. `pread` could avoid this issue but it's not supported on windows. [0]: https://www.sublimetext.com/blog/articles/use-mmap-with-care --- DOCS/man/input.rst | 27 +++++++++++++--------- player/command.c | 56 ++++++++++++++++++++++++++++++---------------- 2 files changed, 53 insertions(+), 30 deletions(-) diff --git a/DOCS/man/input.rst b/DOCS/man/input.rst index 3602a3a01e..7eea15fbcb 100644 --- a/DOCS/man/input.rst +++ b/DOCS/man/input.rst @@ -932,8 +932,9 @@ OSD Commands ``file`` specifies the file the raw image data is read from. It can be either a numeric UNIX file descriptor prefixed with ``@`` (e.g. ``@4``), - or a filename. The file will be mapped into memory with ``mmap()``, - copied, and unmapped before the command returns (changed in mpv 0.18.1). + or a filename. The file will be read into memory before the command returns. + Note that since mpv 0.42.0, if a file descriptor is specified, the file + offset of the descriptor will be modified. It is also possible to pass a raw memory address for use as bitmap memory by passing a memory address as integer prefixed with an ``&`` character. @@ -942,10 +943,6 @@ OSD Commands memory address (since mpv 0.8.0, ignored before). ``offset`` is the byte offset of the first pixel in the source file. - (The current implementation always mmap's the whole file from position 0 to - the end of the image, so large offsets should be avoided. Before mpv 0.8.0, - the offset was actually passed directly to ``mmap``, but it was changed to - make using it easier.) ``fmt`` is a string identifying the image format. Currently, only ``bgra`` is defined. This format has 4 bytes per pixels, with 8 bits per component. @@ -973,12 +970,20 @@ OSD Commands .. note:: + Before mpv 0.42.0, the file was mapped into memory with ``mmap()``, + copied, and unmapped before the command returns. Changing the file + before the command returns is highly discouraged as this could lead to + crashes on versions before 0.42.0. + Before mpv 0.18.1, you had to do manual "double buffering" when updating - an overlay by replacing it with a different memory buffer. Since mpv - 0.18.1, the memory is simply copied and doesn't reference any of the - memory indicated by the command's arguments after the command returns. - If you want to use this command before mpv 0.18.1, reads the old docs - to see how to handle this correctly. + an overlay by replacing it with a different memory buffer. If you want + to use this command before mpv 0.18.1, read the old docs to see how to + handle this correctly. + + Larger offsets should be avoided as mpv versions before 0.42.0 used to + mmap the whole file from position 0 to the end of the image. Before mpv + 0.8.0, the offset was actually passed directly to ``mmap``. Since mpv + 0.42.0, no mmap happens, so offset is handled by seeking on the file. ``overlay-remove `` Remove an overlay added with ``overlay-add`` and the same ID. Does nothing diff --git a/player/command.c b/player/command.c index cfa493dc10..eaf392e6e7 100644 --- a/player/command.c +++ b/player/command.c @@ -5152,6 +5152,28 @@ static void replace_overlay(struct MPContext *mpctx, int id, struct overlay *new recreate_overlays(mpctx); } +static bool +fread_pic(FILE *fp, mp_image_t *dst, size_t bytesPerLine, size_t h, size_t stride, size_t offset) +{ + size_t bytesRead = 0, expectedBytes = h * bytesPerLine; + size_t src_off = offset, dst_off = 0; + if (stride == dst->stride[0]) { + fseek(fp, src_off, SEEK_SET); + bytesRead = fread(dst->planes[0], 1, expectedBytes, fp); + } else { + for (int i = 0; i < h; i++) { + fseek(fp, src_off, SEEK_SET); + size_t r = fread(dst->planes[0] + dst_off, 1, bytesPerLine, fp); + if (r != bytesPerLine) + break; + bytesRead += bytesPerLine; + src_off += stride; + dst_off += dst->stride[0]; + } + } + return bytesRead == expectedBytes; +} + static void cmd_overlay_add(void *pcmd) { struct mp_cmd_ctx *cmd = pcmd; @@ -5189,14 +5211,12 @@ static void cmd_overlay_add(void *pcmd) if (!overlay.source) goto error; int fd = -1; - bool close_fd = true; void *p = NULL; if (file[0] == '@') { char *end; - fd = strtol(&file[1], &end, 10); - if (!file[1] || end[0]) - fd = -1; - close_fd = false; + int srcfd = strtol(&file[1], &end, 10); + if (file[1] && end[0] == '\0' && srcfd >= 0) + fd = mp_dup_cloexec(srcfd); // fdopen will "own" the fd } else if (file[0] == '&') { char *end; unsigned long long addr = strtoull(&file[1], &end, 0); @@ -5206,24 +5226,22 @@ static void cmd_overlay_add(void *pcmd) } else { fd = open(file, O_RDONLY | O_BINARY | O_CLOEXEC); } - size_t map_size = 0; - if (fd >= 0) { - map_size = offset + h * stride; - void *m = mmap(NULL, map_size, PROT_READ, MAP_SHARED, fd, 0); - if (close_fd) - close(fd); - if (m && m != MAP_FAILED) - p = m; + + FILE *fp = (fd >= 0) ? fdopen(fd, "rb") : NULL; + int open_or_read_err = 1, bytesPerLine = w * 4; + if (p) { + memcpy_pic(overlay.source->planes[0], (char *)p + offset, bytesPerLine, + h, overlay.source->stride[0], stride); + open_or_read_err = 0; + } else if (fp) { + open_or_read_err = !fread_pic(fp, overlay.source, bytesPerLine, h, stride, offset); + fclose(fp); } - if (!p) { - MP_ERR(mpctx, "overlay-add: could not open or map '%s'\n", file); + if (open_or_read_err) { + MP_ERR(mpctx, "overlay-add: could not open or read '%s'\n", file); talloc_free(overlay.source); goto error; } - memcpy_pic(overlay.source->planes[0], (char *)p + offset, w * 4, h, - overlay.source->stride[0], stride); - if (map_size) - munmap(p, map_size); replace_overlay(mpctx, id, &overlay); return;