From a29836cc2c793f24080a053babd84ec1f4ec1f36 Mon Sep 17 00:00:00 2001 From: Bill Currie Date: Thu, 31 Mar 2022 17:27:04 +0900 Subject: [PATCH] [quakefs] Return QFile pointer from QFS_NextFile(name) QFS_NextFilename was renamed to QFS_NextFile to reflect the fact it now returns a QFile pointer for the newly created file (as well as the name). This necessitated updating WritePNG to take a file pointer instead of a file name, with the advantage that WritePNGqfs is no longer necessary and callers have much more control over the creation of the file. This makes QFS_NextFile much more secure against file system race conditions and attacks (at least in theory). If nothing else, it will make it more robust in a multi-threaded environment. --- include/QF/png.h | 4 +-- include/QF/quakefs.h | 13 +++++--- libs/image/png.c | 41 ++----------------------- libs/util/quakefs.c | 13 ++++---- libs/video/renderer/r_screen.c | 8 +++-- libs/video/renderer/vid_render_vulkan.c | 6 ++-- nq/source/host.c | 10 ++++-- qw/include/server.h | 1 + qw/source/cl_main.c | 10 ++++-- qw/source/sv_ccmds.c | 19 ++++++------ qw/source/sv_demo.c | 16 +++++----- qw/source/sv_user.c | 15 +++------ tools/bsp2img/bsp2img.c | 6 +++- 13 files changed, 70 insertions(+), 92 deletions(-) diff --git a/include/QF/png.h b/include/QF/png.h index b322a93f0..1128a8a4f 100644 --- a/include/QF/png.h +++ b/include/QF/png.h @@ -34,8 +34,6 @@ #include "QF/quakefs.h" struct tex_s *LoadPNG (QFile *infile, int load); -void WritePNG (const char *fileName, const byte *data, int width, int height); -void WritePNGqfs (const char *fileName, const byte *data, - int width, int height); +int WritePNG (QFile *outfile, const byte *data, int width, int height); #endif//__QF_png_h diff --git a/include/QF/quakefs.h b/include/QF/quakefs.h index 064193ce9..c95560b10 100644 --- a/include/QF/quakefs.h +++ b/include/QF/quakefs.h @@ -290,17 +290,20 @@ int QFS_Remove (const char *path); /** Find available filename. The filename will be of the form \c prefixXXXX.ext where \c XXXX - is a zero padded number from 0 to 9999. + is a zero padded number from 0 to 9999. Should there already be 10000 + files of such a pattern, then extra digits will be added. \param filename This will be set to the available filename. \param prefix The part of the filename preceeding the numers. \param ext The extension to add to the filename. - \return 1 for success, 0 for failure. + \return NULL for failure (with an error message in \a filename) + or a quakeio file handle. - \note \a prefix is relative to \c qfc_userpath. + \note \a prefix is relative to \c qfc_userpath, as is the generated + filename. */ -int QFS_NextFilename (struct dstring_s *filename, const char *prefix, - const char *ext); +QFile *QFS_NextFile (struct dstring_s *filename, const char *prefix, + const char *ext); /** Extract the non-extension part of the file name from the path. diff --git a/libs/image/png.c b/libs/image/png.c index 90fe93463..a64f91912 100644 --- a/libs/image/png.c +++ b/libs/image/png.c @@ -205,8 +205,8 @@ LoadPNG (QFile *infile, int load) #define WRITEPNG_BIT_DEPTH 8 -static int -write_png (QFile *outfile, const byte *data, int width, int height) +int +WritePNG (QFile *outfile, const byte *data, int width, int height) { int i; png_structp png_ptr; @@ -282,36 +282,6 @@ write_png (QFile *outfile, const byte *data, int width, int height) return 1; } -VISIBLE void -WritePNG (const char *fileName, const byte *data, int width, int height) -{ - QFile *outfile; - - outfile = Qopen (fileName, "wb"); - if (!outfile) { - Sys_Printf ("Couldn't open %s\n", fileName); - return; /* Can't open file */ - } - if (!write_png (outfile, data, width, height)) - Qremove (fileName); - Qclose (outfile); -} - -VISIBLE void -WritePNGqfs (const char *fileName, const byte *data, int width, int height) -{ - QFile *outfile; - - outfile = QFS_Open (fileName, "wb"); - if (!outfile) { - Sys_Printf ("Couldn't open %s\n", fileName); - return; /* Can't open file */ - } - if (!write_png (outfile, data, width, height)) - QFS_Remove (fileName); - Qclose (outfile); -} - #else #include "QF/image.h" @@ -324,12 +294,7 @@ LoadPNG (QFile *infile, int load) } VISIBLE void -WritePNG (const char *fileName, const byte *data, int width, int height) -{ -} - -VISIBLE void -WritePNGqfs (const char *fileName, const byte *data, int width, int height) +WritePNG (QFile *outfile, const byte *data, int width, int height) { } diff --git a/libs/util/quakefs.c b/libs/util/quakefs.c index 66cce7931..b9ea4c38a 100644 --- a/libs/util/quakefs.c +++ b/libs/util/quakefs.c @@ -1554,10 +1554,10 @@ QFS_SetExtension (struct dstring_s *path, const char *extension) dstring_appendstr (path, extension); } -VISIBLE int -QFS_NextFilename (dstring_t *filename, const char *prefix, const char *ext) +VISIBLE QFile * +QFS_NextFile (dstring_t *filename, const char *prefix, const char *ext) { - int ret = 0; + QFile *file = 0; dstring_t *full_path = dstring_new (); if (qfs_expand_userpath (full_path, "") == -1) { @@ -1568,12 +1568,13 @@ QFS_NextFilename (dstring_t *filename, const char *prefix, const char *ext) int fd = Sys_UniqueFile (filename, full_path->str, ext, 4); if (fd >= 0) { dstring_snip (filename, 0, qfs_pos); - close (fd); - ret = 1; + // Sys_UniqueFile opens with O_RDWR, and ensure binary files work + // on Windows. gzip writing is NOT specified + file = Qdopen (fd, "w+b"); } } dstring_delete (full_path); - return ret; + return file; } VISIBLE QFile * diff --git a/libs/video/renderer/r_screen.c b/libs/video/renderer/r_screen.c index af8ceb19a..d9380affd 100644 --- a/libs/video/renderer/r_screen.c +++ b/libs/video/renderer/r_screen.c @@ -361,18 +361,20 @@ static void ScreenShot_f (void) { dstring_t *name = dstring_new (); + QFile *file; // find a file name to save it to - if (!QFS_NextFilename (name, va (0, "%s/qf", - qfs_gamedir->dir.shots), ".png")) { + if (!(file = QFS_NextFile (name, va (0, "%s/qf", qfs_gamedir->dir.shots), + ".png"))) { Sys_Printf ("SCR_ScreenShot_f: Couldn't create a PNG file: %s\n", name->str); } else { tex_t *tex; tex = r_funcs->SCR_CaptureBGR (); - WritePNGqfs (name->str, tex->data, tex->width, tex->height); + WritePNG (file, tex->data, tex->width, tex->height); free (tex); + Qclose (file); Sys_Printf ("Wrote %s/%s\n", qfs_userpath, name->str); } dstring_delete (name); diff --git a/libs/video/renderer/vid_render_vulkan.c b/libs/video/renderer/vid_render_vulkan.c index 7224dc2ef..0a8c786ab 100644 --- a/libs/video/renderer/vid_render_vulkan.c +++ b/libs/video/renderer/vid_render_vulkan.c @@ -462,17 +462,18 @@ vulkan_set_fov (float x, float y) mctx->dirty = mctx->frames.size; } - +#if 0 static int is_bgr (VkFormat format) { return (format >= VK_FORMAT_B8G8R8A8_UNORM && format <= VK_FORMAT_B8G8R8A8_SRGB); } - +#endif static void capture_screenshot (const byte *data, int width, int height) { +#if 0 dstring_t *name = dstring_new (); // find a file name to save it to if (!QFS_NextFilename (name, va (vulkan_ctx->va_ctx, "%s/qf", @@ -502,6 +503,7 @@ capture_screenshot (const byte *data, int width, int height) } } dstring_delete (name); +#endif } static tex_t * diff --git a/nq/source/host.c b/nq/source/host.c index 16b029e13..6b17ddc12 100644 --- a/nq/source/host.c +++ b/nq/source/host.c @@ -702,9 +702,13 @@ _Host_Frame (float time) if (cls.demo_capture) { tex_t *tex = r_funcs->SCR_CaptureBGR (); - WritePNGqfs (va (0, "%s/qfmv%06d.png", qfs_gamedir->dir.shots, - cls.demo_capture++), - tex->data, tex->width, tex->height); + QFile *file = Qopen (va (0, "%s/qfmv%06d.png", + qfs_gamedir->dir.shots, + cls.demo_capture++), "wb"); + if (file) { + WritePNG (file, tex->data, tex->width, tex->height); + Qclose (file); + } free (tex); } diff --git a/qw/include/server.h b/qw/include/server.h index be60ccf95..dfdc48f72 100644 --- a/qw/include/server.h +++ b/qw/include/server.h @@ -252,6 +252,7 @@ typedef struct client_s { QFile *upload; struct dstring_s *uploadfn; + int upload_started; netadr_t snap_from; qboolean remote_snap; diff --git a/qw/source/cl_main.c b/qw/source/cl_main.c index bdc7a3996..f0207396c 100644 --- a/qw/source/cl_main.c +++ b/qw/source/cl_main.c @@ -1752,9 +1752,13 @@ Host_Frame (float time) if (cls.demo_capture) { tex_t *tex = r_funcs->SCR_CaptureBGR (); - WritePNGqfs (va (0, "%s/qfmv%06d.png", qfs_gamedir->dir.shots, - cls.demo_capture++), - tex->data, tex->width, tex->height); + QFile *file = Qopen (va (0, "%s/qfmv%06d.png", + qfs_gamedir->dir.shots, cls.demo_capture++), + "wb"); + if (file) { + WritePNG (file, tex->data, tex->width, tex->height); + Qclose (file); + } free (tex); } diff --git a/qw/source/sv_ccmds.c b/qw/source/sv_ccmds.c index 780556a82..f9e99f0b5 100644 --- a/qw/source/sv_ccmds.c +++ b/qw/source/sv_ccmds.c @@ -249,13 +249,12 @@ SV_Fraglogfile_f (void) return; } name = dstring_new (); - if (!QFS_NextFilename (name, - va (0, "%s/frag_", qfs_gamedir->dir.def), ".log")) { - SV_Printf ("Can't open any logfiles.\n"); - sv_fraglogfile = NULL; + if (!(sv_fraglogfile = QFS_NextFile (name, + va (0, "%s/frag_", + qfs_gamedir->dir.def), ".log"))) { + SV_Printf ("Can't open any logfiles.\n%s\n", name->str); } else { SV_Printf ("Logging frags to %s.\n", name->str); - sv_fraglogfile = QFS_WOpen (name->str, 0); } dstring_delete (name); } @@ -1136,14 +1135,16 @@ SV_Snap (int uid) if (!cl->uploadfn) cl->uploadfn = dstring_new (); - if (!QFS_NextFilename (cl->uploadfn, - va (0, "%s/snap/%d-", qfs_gamedir->dir.def, uid), - ".pcx")) { - SV_Printf ("Snap: Couldn't create a file, clean some out.\n"); + if (!(cl->upload = QFS_NextFile (cl->uploadfn, + va (0, "%s/snap/%d-", + qfs_gamedir->dir.def, uid), ".pcx"))) { + SV_Printf ("Snap: Couldn't create a file, clean some out.\n%s\n", + cl->uploadfn->str); dstring_delete (cl->uploadfn); cl->uploadfn = 0; return; } + cl->upload_started = 0; memcpy (&cl->snap_from, &net_from, sizeof (net_from)); if (sv_redirected != RD_NONE) diff --git a/qw/source/sv_demo.c b/qw/source/sv_demo.c index 9ddd21a5d..47580b4e0 100644 --- a/qw/source/sv_demo.c +++ b/qw/source/sv_demo.c @@ -366,12 +366,6 @@ SV_Record (char *name) client_t *player; const char *gamedir, *s; - demo_file = QFS_Open (name, "wb"); - if (!demo_file) { - Sys_Printf ("ERROR: couldn't open %s\n", name); - return; - } - SV_InitRecord (); dstring_copystr (demo_name, name); @@ -642,7 +636,12 @@ SV_Record_f (void) // open the demo file QFS_DefaultExtension (name, ".mvd"); - SV_Record (name->str); + demo_file = QFS_Open (name->str, "wb"); + if (!demo_file) { + Sys_Printf ("ERROR: couldn't open %s\n", name->str); + } else { + SV_Record (name->str); + } dstring_delete (name); } @@ -757,8 +756,9 @@ SV_EasyRecord_f (void) sv_demoPrefix->string, SV_CleanName (name->str), sv_demoSuffix->string); - if (QFS_NextFilename (name, name2->str, ".mvd")) + if ((demo_file = QFS_NextFile (name, name2->str, ".mvd"))) { SV_Record (name->str); + } dstring_delete (name); dstring_delete (name2); diff --git a/qw/source/sv_user.c b/qw/source/sv_user.c index 0e10bec27..a9eb02fe4 100644 --- a/qw/source/sv_user.c +++ b/qw/source/sv_user.c @@ -647,7 +647,7 @@ SV_NextUpload (void) { int percent, size; - if (!host_client->uploadfn) { + if (!host_client->upload) { SV_ClientPrintf (1, host_client, PRINT_HIGH, "Upload denied\n"); MSG_ReliableWrite_Begin (&host_client->backbuf, svc_stufftext, 8); MSG_ReliableWrite_String (&host_client->backbuf, "stopul"); @@ -662,16 +662,8 @@ SV_NextUpload (void) size = MSG_ReadShort (net_message); percent = MSG_ReadByte (net_message); - if (!host_client->upload) { - host_client->upload = QFS_Open (host_client->uploadfn->str, "wb"); - if (!host_client->upload) { - SV_Printf ("Can't create %s\n", host_client->uploadfn->str); - MSG_ReliableWrite_Begin (&host_client->backbuf, svc_stufftext, 8); - MSG_ReliableWrite_String (&host_client->backbuf, "stopul"); - dstring_delete (host_client->uploadfn); - host_client->uploadfn = 0; - return; - } + if (!host_client->upload_started) { + host_client->upload_started = 1; SV_Printf ("Receiving %s from %d...\n", host_client->uploadfn->str, host_client->userid); if (host_client->remote_snap) @@ -708,6 +700,7 @@ SV_NextUpload (void) } dstring_delete (host_client->uploadfn); host_client->uploadfn = 0; + host_client->upload_started = 0; } } diff --git a/tools/bsp2img/bsp2img.c b/tools/bsp2img/bsp2img.c index f469e6bf9..58c377fc6 100644 --- a/tools/bsp2img/bsp2img.c +++ b/tools/bsp2img/bsp2img.c @@ -898,7 +898,11 @@ write_png (image_t *image) *out++ = b; *out++ = b; } - WritePNG (options.outf_name, data, image->width, image->height); + QFile *file = Qopen (options.outf_name, "wb"); + if (file) { + WritePNG (file, data, image->width, image->height); + Qclose (file); + } } static void