image_writer: fix TOCTOU in screenshot filename generation

The screenshot command is documented to not overwrite existing files. However, there is a race window between the filename is generated with gen_fname and when the file is open to write. Specifically, the convert_image function in this window can be very time consuming depending on video and screenshot image format and size. This results in existing file being overwritten because the file writing functions don't check for the existance of file. Fix this be opening the file in exclusive mode. Add overwrite parameter to write_image for other operations that are documented to overwrite existing files, like screenshot-to-file. Note that for write_avif, checking existance is used instead because avio_open does not support exclusive open mode.

nanahi committed 2 years ago

Commit 9a861c91 parent aa08e305 files · +32 −16Browse files

Unified · Split

5 files

player/screenshot.c +5 −4
test/scale_test.c +1 −1
video/image_writer.c +24 −9
video/image_writer.h +1 −1
video/out/vo_image.c +1 −1

player/screenshot.c+5−4

 }
 static bool write_screenshot(struct mp_cmd_ctx *cmd, struct mp_image *img,
-                             const char *filename, struct image_writer_opts *opts)
+                             const char *filename, struct image_writer_opts *opts,
+                             bool overwrite)
 {
     struct MPContext *mpctx = cmd->mpctx;
     struct image_writer_opts *gopts = mpctx->opts->screenshot_image_opts;
     mp_core_unlock(mpctx);
     bool ok = img && write_image(img, &opts_copy, filename, mpctx->global,
-                                 mpctx->screenshot_ctx->log);
+                                 mpctx->screenshot_ctx->log, overwrite);
     mp_core_lock(mpctx);
         cmd->success = false;
         return;
     }
-    cmd->success = write_screenshot(cmd, image, filename, &opts);
+    cmd->success = write_screenshot(cmd, image, filename, &opts, true);
     talloc_free(image);
 }
     if (image) {
         char *filename = gen_fname(cmd, image_writer_file_ext(opts));
         if (filename) {
-            cmd->success = write_screenshot(cmd, image, filename, NULL);
+            cmd->success = write_screenshot(cmd, image, filename, NULL, false);
             if (cmd->success) {
                 node_init(res, MPV_FORMAT_NODE_MAP, NULL);
                 node_map_add_string(res, "filename", filename);

test/scale_test.c+1−1

@@ -60,7 +60,7 @@ static void dump_image(struct scale_test stest, const char name, · 59 unmodified lines
60	struct image_writer_opts opts = image_writer_opts_defaults;	60	struct image_writer_opts opts = image_writer_opts_defaults;
61	opts.format = AV_CODEC_ID_PNG;	61	opts.format = AV_CODEC_ID_PNG;
62		62
63	if (!write_image(img, &opts, path, NULL, NULL)) {	63	if (!write_image(img, &opts, path, NULL, NULL, true)) {
64	printf("Failed to write '%s'.\n", path);	64	printf("Failed to write '%s'.\n", path);
65	abort();	65	abort();
66	}	66	}

video/image_writer.c+24−9

 #endif
 #include "osdep/io.h"
+#include "misc/path_utils.h"
 #include "common/av_common.h"
 #include "common/msg.h"
     );
 }
-static bool write_lavc(struct image_writer_ctx *ctx, mp_image_t *image, const char *filename)
+static bool write_lavc(struct image_writer_ctx *ctx, mp_image_t *image,
+                       const char *filename, bool overwrite)
 {
-    FILE *fp = fopen(filename, "wb");
+    FILE *fp = fopen(filename, overwrite ? "wb" : "wbx");
+    if (!fp && errno == EEXIST) {
+        MP_ERR(ctx, "File '%s' already exists!\n", filename);
+        return false;
+    }
     if (!fp) {
         MP_ERR(ctx, "Error opening '%s' for writing!\n", filename);
         return false;
 }
 static bool write_jpeg(struct image_writer_ctx *ctx, mp_image_t *image,
-                       const char *filename)
+                       const char *filename, bool overwrite)
 {
-    FILE *fp = fopen(filename, "wb");
+    FILE *fp = fopen(filename, overwrite ? "wb" : "wbx");
+    if (!fp && errno == EEXIST) {
+        MP_ERR(ctx, "File '%s' already exists!\n", filename);
+        return false;
+    }
     if (!fp) {
         MP_ERR(ctx, "Error opening '%s' for writing!\n", filename);
         return false;
 }
 static bool write_avif(struct image_writer_ctx *ctx, mp_image_t *image,
-                       const char *filename)
+                       const char *filename, bool overwrite)
 {
     const AVCodec *codec = NULL;
     const AVOutputFormat *ofmt = NULL;
         goto free_data;
     }
+    if (!overwrite && mp_path_exists(filename)) {
+        MP_ERR(ctx, "File '%s' already exists\n", filename);
+        goto free_data;
+    }
     ret = avio_open(&avioctx, filename, AVIO_FLAG_WRITE);
     if (ret < 0) {
         MP_ERR(ctx, "Could not open file '%s' for saving images\n", filename);
 bool write_image(struct mp_image *image, const struct image_writer_opts *opts,
                  const char *filename, struct mpv_global *global,
-                 struct mp_log *log)
+                 struct mp_log *log, bool overwrite)
 {
     struct image_writer_opts defs = image_writer_opts_defaults;
     if (!opts)
     mp_verbose(log, "input: %s\n", mp_image_params_to_str(&image->params));
     struct image_writer_ctx ctx = { log, opts, image->fmt };
-    bool (*write)(struct image_writer_ctx *, mp_image_t *, const char *) = write_lavc;
+    bool (*write)(struct image_writer_ctx *, mp_image_t *, const char *, bool) = write_lavc;
     int destfmt = 0;
 #if HAVE_JPEG
     if (!dst)
         return false;
-    bool success = write(&ctx, dst, filename);
+    bool success = write(&ctx, dst, filename, overwrite);
     if (!success)
         mp_err(log, "Error writing file '%s'!\n", filename);
 {
     struct image_writer_opts opts = image_writer_opts_defaults;
     opts.format = AV_CODEC_ID_PNG;
-    write_image(image, &opts, filename, NULL, log);
+    write_image(image, &opts, filename, NULL, log, true);
 }

video/image_writer.h+1−1

@@ -68,7 +68,7 @@ int image_writer_format_from_ext(const char *ext); · 67 unmodified lines
68	*/	68	*/
69	bool write_image(struct mp_image image, const struct image_writer_opts opts,	69	bool write_image(struct mp_image image, const struct image_writer_opts opts,
70	const char filename, struct mpv_global global,	70	const char filename, struct mpv_global global,
71	struct mp_log *log);	71	struct mp_log *log, bool overwrite);
72		72
73	// Debugging helper.	73	// Debugging helper.
74	void dump_png(struct mp_image image, const char filename, struct mp_log *log);	74	void dump_png(struct mp_image image, const char filename, struct mp_log *log);

video/out/vo_image.c+1−1

@@ -118,7 +118,7 @@ static void flip_page(struct vo *vo) · 117 unmodified lines
118	filename = mp_path_join(t, p->opts->outdir, filename);	118	filename = mp_path_join(t, p->opts->outdir, filename);
119		119
120	MP_INFO(vo, "Saving %s\n", filename);	120	MP_INFO(vo, "Saving %s\n", filename);
121	write_image(p->current, p->opts->opts, filename, vo->global, vo->log);	121	write_image(p->current, p->opts->opts, filename, vo->global, vo->log, true);
122		122
123	talloc_free(t);	123	talloc_free(t);
124	}	124	}

image_writer: fix TOCTOU in screenshot filename generation

Keyboard shortcuts

General

Issues and pull requests

Issues, pull requests and Inbox

Repository code