Re: [PATCH v2] block: Refactor get_tmp_filename()

[Markus Armbruster <armbru@redhat.com>]

Bin Meng <bmeng.cn@gmail.com> writes:

> On Mon, Sep 26, 2022 at 6:13 PM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Bin Meng <bmeng.cn@gmail.com> writes:
>>
>> > From: Bin Meng <bin.meng@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.meng@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.

Care to fix that?

>> 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. :(

Looks like it requires surgery to bdrv_create() at least.  I'm not
demanding you do that now.

[...]