| Message ID | 1439939415-18180-4-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 08/18/2015 05:10 PM, Max Reitz wrote: > Split the part which actually refreshes the BlockDriverState.filename > field off of bdrv_refresh_filename() into a more generic function > bdrv_filename(), which first calls bdrv_refresh_filename() and then > stores a qemu-usable filename into the given buffer instead of > BlockDriverState.filename. > > Since bdrv_refresh_filename() therefore no longer refreshes that field, > some calls to that function have to be replaced by calls to > bdrv_filename() "manually" refreshing the BDS filename field (this is > only temporary). > > Additionally, a wrapper function bdrv_filename_alloc() is added which > allocates a buffer of size PATH_MAX, call bdrv_filename() on that buffer > and returns it, since needing a temporary buffer for the filename is a > rather common pattern. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block.c | 39 ++++++++++++++++++++++++++++++++------- > block/blkverify.c | 3 ++- > block/quorum.c | 2 +- > include/block/block.h | 2 ++ > 4 files changed, 37 insertions(+), 9 deletions(-) > > +/* First refreshes exact_filename and full_open_options by calling > + * bdrv_refresh_filename(). Then, if exact_filename is set, it is copied into > + * the target buffer. Otherwise, full_open_options is converted to a JSON > + * object, prefixed with "json:" (for use through the JSON pseudo protocol) and > + * put there. > + * > + * If sz > 0, the string put into the buffer will always be null-terminated. > + * > + * Returns @dest. > + */ > +char *bdrv_filename(BlockDriverState *bs, char *dest, size_t sz) > +{ How does one tell if 'sz' was large enough, vs. too short and therefore the snprintf() truncated the resulting string? Would the code be any simpler if this always returned a freshly g_malloc'd string of the right length, rather than making the caller have to pre-allocate and guess whether the allocation was big enough? > + bdrv_refresh_filename(bs); > + > + if (sz > INT_MAX) { > + sz = INT_MAX; > + } > > if (bs->exact_filename[0]) { > - pstrcpy(bs->filename, sizeof(bs->filename), bs->exact_filename); > + pstrcpy(dest, sz, bs->exact_filename); > } else if (bs->full_open_options) { > QString *json = qobject_to_json(QOBJECT(bs->full_open_options)); > - snprintf(bs->filename, sizeof(bs->filename), "json:%s", > - qstring_get_str(json)); > + snprintf(dest, sz, "json:%s", qstring_get_str(json)); > QDECREF(json); > } > + > + return dest; In other words, I think it's very dangerous to use snprintf() without checking whether the result fit.
On 31.08.2015 23:00, Eric Blake wrote: > On 08/18/2015 05:10 PM, Max Reitz wrote: >> Split the part which actually refreshes the BlockDriverState.filename >> field off of bdrv_refresh_filename() into a more generic function >> bdrv_filename(), which first calls bdrv_refresh_filename() and then >> stores a qemu-usable filename into the given buffer instead of >> BlockDriverState.filename. >> >> Since bdrv_refresh_filename() therefore no longer refreshes that field, >> some calls to that function have to be replaced by calls to >> bdrv_filename() "manually" refreshing the BDS filename field (this is >> only temporary). >> >> Additionally, a wrapper function bdrv_filename_alloc() is added which >> allocates a buffer of size PATH_MAX, call bdrv_filename() on that buffer >> and returns it, since needing a temporary buffer for the filename is a >> rather common pattern. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> block.c | 39 ++++++++++++++++++++++++++++++++------- >> block/blkverify.c | 3 ++- >> block/quorum.c | 2 +- >> include/block/block.h | 2 ++ >> 4 files changed, 37 insertions(+), 9 deletions(-) >> > >> +/* First refreshes exact_filename and full_open_options by calling >> + * bdrv_refresh_filename(). Then, if exact_filename is set, it is copied into >> + * the target buffer. Otherwise, full_open_options is converted to a JSON >> + * object, prefixed with "json:" (for use through the JSON pseudo protocol) and >> + * put there. >> + * >> + * If sz > 0, the string put into the buffer will always be null-terminated. >> + * >> + * Returns @dest. >> + */ >> +char *bdrv_filename(BlockDriverState *bs, char *dest, size_t sz) >> +{ > > How does one tell if 'sz' was large enough, vs. too short and therefore > the snprintf() truncated the resulting string? Would the code be any > simpler if this always returned a freshly g_malloc'd string of the right > length, rather than making the caller have to pre-allocate and guess > whether the allocation was big enough? > >> + bdrv_refresh_filename(bs); >> + >> + if (sz > INT_MAX) { >> + sz = INT_MAX; >> + } >> >> if (bs->exact_filename[0]) { >> - pstrcpy(bs->filename, sizeof(bs->filename), bs->exact_filename); >> + pstrcpy(dest, sz, bs->exact_filename); >> } else if (bs->full_open_options) { >> QString *json = qobject_to_json(QOBJECT(bs->full_open_options)); >> - snprintf(bs->filename, sizeof(bs->filename), "json:%s", >> - qstring_get_str(json)); >> + snprintf(dest, sz, "json:%s", qstring_get_str(json)); >> QDECREF(json); >> } >> + >> + return dest; > > In other words, I think it's very dangerous to use snprintf() without > checking whether the result fit. There are a couple of places in qemu which copy BDS.filename into a pre-existing buffer, so I'd rather not just drop bdrv_filename() as it is. I guess I'll just make it so that calling bdrv_filename() with a NULL dest will result in the buffer being allocated. Note however that there are a couple of places in qemu which rely on filenames not exceeding PATH_MAX (by using PATH_MAX sized buffers for holding them). Maybe we'll eventually get around to fix them, but right now it's a limitation not introduced by this series. Max
diff --git a/block.c b/block.c index 1572785..d2a3e8d 100644 --- a/block.c +++ b/block.c @@ -1559,7 +1559,7 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename, } } - bdrv_refresh_filename(bs); + bdrv_filename(bs, bs->filename, sizeof(bs->filename)); /* For snapshot=on, create a temporary qcow2 overlay. bs points to the * temporary snapshot afterwards. */ @@ -4153,9 +4153,6 @@ static bool append_open_options(QDict *d, BlockDriverState *bs) * - full_open_options: Options which, when given when opening a block device * (without a filename), result in a BDS (mostly) * equalling the given one - * - filename: If exact_filename is set, it is copied here. Otherwise, - * full_open_options is converted to a JSON object, prefixed with - * "json:" (for use through the JSON pseudo protocol) and put here. */ void bdrv_refresh_filename(BlockDriverState *bs) { @@ -4242,15 +4239,43 @@ void bdrv_refresh_filename(BlockDriverState *bs) bs->full_open_options = opts; } +} + +/* First refreshes exact_filename and full_open_options by calling + * bdrv_refresh_filename(). Then, if exact_filename is set, it is copied into + * the target buffer. Otherwise, full_open_options is converted to a JSON + * object, prefixed with "json:" (for use through the JSON pseudo protocol) and + * put there. + * + * If sz > 0, the string put into the buffer will always be null-terminated. + * + * Returns @dest. + */ +char *bdrv_filename(BlockDriverState *bs, char *dest, size_t sz) +{ + bdrv_refresh_filename(bs); + + if (sz > INT_MAX) { + sz = INT_MAX; + } if (bs->exact_filename[0]) { - pstrcpy(bs->filename, sizeof(bs->filename), bs->exact_filename); + pstrcpy(dest, sz, bs->exact_filename); } else if (bs->full_open_options) { QString *json = qobject_to_json(QOBJECT(bs->full_open_options)); - snprintf(bs->filename, sizeof(bs->filename), "json:%s", - qstring_get_str(json)); + snprintf(dest, sz, "json:%s", qstring_get_str(json)); QDECREF(json); } + + return dest; +} + +/* Wrapper around bdrv_filename() using g_malloc(PATH_MAX) for the destination + * buffer. + */ +char *bdrv_filename_alloc(BlockDriverState *bs) +{ + return bdrv_filename(bs, g_malloc(PATH_MAX), PATH_MAX); } /* This accessor function purpose is to allow the device models to access the diff --git a/block/blkverify.c b/block/blkverify.c index d277e63..cb9ce02 100644 --- a/block/blkverify.c +++ b/block/blkverify.c @@ -309,7 +309,8 @@ static void blkverify_refresh_filename(BlockDriverState *bs) BDRVBlkverifyState *s = bs->opaque; /* bs->file has already been refreshed */ - bdrv_refresh_filename(s->test_file); + bdrv_filename(s->test_file, s->test_file->filename, + sizeof(s->test_file->filename)); if (bs->file->full_open_options && s->test_file->full_open_options) { QDict *opts = qdict_new(); diff --git a/block/quorum.c b/block/quorum.c index 2f6c45f..00d1fb0 100644 --- a/block/quorum.c +++ b/block/quorum.c @@ -1003,7 +1003,7 @@ static void quorum_refresh_filename(BlockDriverState *bs) int i; for (i = 0; i < s->num_children; i++) { - bdrv_refresh_filename(s->bs[i]); + bdrv_filename(s->bs[i], s->bs[i]->filename, sizeof(s->bs[i]->filename)); if (!s->bs[i]->full_open_options) { return; } diff --git a/include/block/block.h b/include/block/block.h index a78e4f1..4be6c18 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -267,6 +267,8 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs, const char *backing_file); int bdrv_get_backing_file_depth(BlockDriverState *bs); void bdrv_refresh_filename(BlockDriverState *bs); +char *bdrv_filename(BlockDriverState *bs, char *dest, size_t sz); +char *bdrv_filename_alloc(BlockDriverState *bs); int bdrv_truncate(BlockDriverState *bs, int64_t offset); int64_t bdrv_nb_sectors(BlockDriverState *bs); int64_t bdrv_getlength(BlockDriverState *bs);
Split the part which actually refreshes the BlockDriverState.filename field off of bdrv_refresh_filename() into a more generic function bdrv_filename(), which first calls bdrv_refresh_filename() and then stores a qemu-usable filename into the given buffer instead of BlockDriverState.filename. Since bdrv_refresh_filename() therefore no longer refreshes that field, some calls to that function have to be replaced by calls to bdrv_filename() "manually" refreshing the BDS filename field (this is only temporary). Additionally, a wrapper function bdrv_filename_alloc() is added which allocates a buffer of size PATH_MAX, call bdrv_filename() on that buffer and returns it, since needing a temporary buffer for the filename is a rather common pattern. Signed-off-by: Max Reitz <mreitz@redhat.com> --- block.c | 39 ++++++++++++++++++++++++++++++++------- block/blkverify.c | 3 ++- block/quorum.c | 2 +- include/block/block.h | 2 ++ 4 files changed, 37 insertions(+), 9 deletions(-)