On Mon, Sep 26, 2022 at 6:13 PM Markus Armbruster <arm...@redhat.com> wrote:
>
> Bin Meng <bmeng...@gmail.com> writes:
>
> > From: Bin Meng <bin.m...@windriver.com>
> >
> > At present there are two callers of get_tmp_filename() and they are
> > inconsistent.
> >
> > One does:
> >
> > /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
> > char *tmp_filename = g_malloc0(PATH_MAX + 1);
> > ...
> > ret = get_tmp_filename(tmp_filename, PATH_MAX + 1);
> >
> > while the other does:
> >
> > s->qcow_filename = g_malloc(PATH_MAX);
> > ret = get_tmp_filename(s->qcow_filename, PATH_MAX);
> >
> > As we can see different 'size' arguments are passed. There are also
> > platform specific implementations inside the function, and this use
> > of snprintf is really undesirable.
> >
> > Refactor this routine by changing its signature to:
> >
> > char *get_tmp_filename(void)
> >
> > and use g_file_open_tmp() for a consistent implementation.
> >
> > Signed-off-by: Bin Meng <bin.m...@windriver.com>
> > ---
> >
> > Changes in v2:
> > - Use g_autofree and g_steal_pointer
> >
> > include/block/block_int-common.h | 2 +-
> > block.c | 42 ++++++++++----------------------
> > block/vvfat.c | 8 +++---
> > 3 files changed, 18 insertions(+), 34 deletions(-)
> >
> > diff --git a/include/block/block_int-common.h
> > b/include/block/block_int-common.h
> > index 8947abab76..ea69a9349c 100644
> > --- a/include/block/block_int-common.h
> > +++ b/include/block/block_int-common.h
> > @@ -1230,7 +1230,7 @@ static inline BlockDriverState *child_bs(BdrvChild
> > *child)
> > }
> >
> > int bdrv_check_request(int64_t offset, int64_t bytes, Error **errp);
> > -int get_tmp_filename(char *filename, int size);
> > +char *get_tmp_filename(void);
> > void bdrv_parse_filename_strip_prefix(const char *filename, const char
> > *prefix,
> > QDict *options);
> >
> > diff --git a/block.c b/block.c
> > index bc85f46eed..4e7a795566 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -860,38 +860,23 @@ int bdrv_probe_geometry(BlockDriverState *bs,
> > HDGeometry *geo)
> >
> > /*
> > * Create a uniquely-named empty temporary file.
> > - * Return 0 upon success, otherwise a negative errno value.
> > + * Return the actual name used upon success, otherwise NULL.
> > + * The called function is responsible for freeing it.
> > */
> > -int get_tmp_filename(char *filename, int size)
> > +char *get_tmp_filename(void)
> > {
> > -#ifdef _WIN32
> > - char temp_dir[MAX_PATH];
> > - /* GetTempFileName requires that its output buffer (4th param)
> > - have length MAX_PATH or greater. */
> > - assert(size >= MAX_PATH);
> > - return (GetTempPath(MAX_PATH, temp_dir)
> > - && GetTempFileName(temp_dir, "qem", 0, filename)
> > - ? 0 : -GetLastError());
> > -#else
> > + g_autofree char *filename = NULL;
> > int fd;
> > - const char *tmpdir;
> > - tmpdir = getenv("TMPDIR");
> > - if (!tmpdir) {
> > - tmpdir = "/var/tmp";
> > - }
> > - if (snprintf(filename, size, "%s/vl.XXXXXX", tmpdir) >= size) {
> > - return -EOVERFLOW;
> > - }
> > - fd = mkstemp(filename);
> > +
> > + fd = g_file_open_tmp("vl.XXXXXX", &filename, NULL);
> > if (fd < 0) {
> > - return -errno;
> > + return NULL;
> > }
> > if (close(fd) != 0) {
> > unlink(filename);
> > - return -errno;
> > + return NULL;
> > }
> > - return 0;
> > -#endif
> > + return g_steal_pointer(&filename);
> > }
>
> Oh my, what a lovely mess you're messing with!
>
> The function creates a temporary *file*, not just a filename. Makes
> sense, as creating a unique filename is inherently racy. The contract
> is clear enough ("Create a uniquely-named empty temporary file"), but
> the function name is actively misleading.
Agreed that the name is misleading.
> Of course, creating a temporary file for the caller to (re)open is also
> racy. By the time the caller gets around to it, the filename could name
> anything. Return an open file file descriptor is a better idea. It's
> basically g_file_open_tmp(). Could we rework the two users of
> get_tmp_filename() accept a file descriptor?
I looked at the 2 callers, and it looks like we need to do more than
these 2 callers to teach them to accept a file descriptor. :(
>
> Anyway, code after the patch:
>
> /*
> * Create a uniquely-named empty temporary file.
> * Return the actual name used upon success, otherwise NULL.
> * The called function is responsible for freeing it.
> */
> char *get_tmp_filename(void)
> {
> g_autofree char *filename = NULL;
> int fd;
>
> fd = g_file_open_tmp("vl.XXXXXX", &filename, NULL);
> if (fd < 0) {
>
> As Philippe wrote, this throws away possibly useful error information.
>
> return NULL;
> }
> if (close(fd) != 0) {
> unlink(filename);
> return NULL;
> }
> return g_steal_pointer(&filename);
> }
>
> Other than that, it's an improvement within the limits of a flawed
> interface.
>
> [...]
>
Regards,
Bin
