Re: [PATCH v3 1/7] block/quorum.c: stable children names

[Max Reitz]

On 04.08.20 12:46, Lukas Straub wrote:
> If we remove the child with the highest index from the quorum,
> decrement s->next_child_index. This way we get stable children
> names as long as we only remove the last child.
> 
> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> Fixes: https://bugs.launchpad.net/bugs/1881231
> Reviewed-by: Zhang Chen <chen.zhang@intel.com>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> ---
>  block/quorum.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/block/quorum.c b/block/quorum.c
> index 6df9449fc2..e846a7e892 100644
> --- a/block/quorum.c
> +++ b/block/quorum.c
> @@ -29,6 +29,8 @@
> 
>  #define HASH_LENGTH 32
> 
> +#define INDEXSTR_LEN 32
> +

I don’t like this too much.  There is no real concept such as an index
string, and it doesn’t have a fixed length.  It just so happens that
there are a couple of places where we need some buffer to snprintf()
into, and all those places happen to use 32 because that’s definitely
sufficient for “children.%u�.  (Technically, it should not be fixed to
32, but “sizeof("children.") + ceil(log_10(INT_MAX))�.)

Whenever we then use such a buffer, we shouldn’t use the same hard-coded
magic number or constant, but instead just refer to the size of the buffer:

>  #define QUORUM_OPT_VOTE_THRESHOLD "vote-threshold"
>  #define QUORUM_OPT_BLKVERIFY      "blkverify"
>  #define QUORUM_OPT_REWRITE        "rewrite-corrupted"
> @@ -970,9 +972,9 @@ static int quorum_open(BlockDriverState *bs, QDict 
> *options, int flags,
>      opened = g_new0(bool, s->num_children);
> 
>      for (i = 0; i < s->num_children; i++) {
> -        char indexstr[32];
> -        ret = snprintf(indexstr, 32, "children.%d", i);
> -        assert(ret < 32);
> +        char indexstr[INDEXSTR_LEN];
> +        ret = snprintf(indexstr, INDEXSTR_LEN, "children.%d", i);
> +        assert(ret < INDEXSTR_LEN);

So in lines 2 and 3 here we should just use sizeof(indexstr).

> 
>          s->children[i] = bdrv_open_child(NULL, options, indexstr, bs,
>                                           &child_of_bds, BDRV_CHILD_DATA, 
> false,
> @@ -1024,7 +1026,7 @@ static void quorum_add_child(BlockDriverState *bs, 
> BlockDriverState *child_bs,
>  {
>      BDRVQuorumState *s = bs->opaque;
>      BdrvChild *child;
> -    char indexstr[32];
> +    char indexstr[INDEXSTR_LEN];
>      int ret;
> 
>      if (s->is_blkverify) {
> @@ -1039,8 +1041,8 @@ static void quorum_add_child(BlockDriverState *bs, 
> BlockDriverState *child_bs,
>          return;
>      }
> 
> -    ret = snprintf(indexstr, 32, "children.%u", s->next_child_index);
> -    if (ret < 0 || ret >= 32) {
> +    ret = snprintf(indexstr, INDEXSTR_LEN, "children.%u", 
> s->next_child_index);
> +    if (ret < 0 || ret >= INDEXSTR_LEN) {

Same here.

>          error_setg(errp, "cannot generate child name");
>          return;
>      }
> @@ -1068,6 +1070,7 @@ static void quorum_del_child(BlockDriverState *bs, 
> BdrvChild *child,
>                               Error **errp)
>  {
>      BDRVQuorumState *s = bs->opaque;
> +    char indexstr[INDEXSTR_LEN];
>      int i;
> 
>      for (i = 0; i < s->num_children; i++) {
> @@ -1089,6 +1092,11 @@ static void quorum_del_child(BlockDriverState *bs, 
> BdrvChild *child,
>      /* We know now that num_children > threshold, so blkverify must be false 
> */
>      assert(!s->is_blkverify);
> 
> +    snprintf(indexstr, INDEXSTR_LEN, "children.%u", s->next_child_index - 1);
> +    if (!strncmp(child->name, indexstr, INDEXSTR_LEN)) {

And here.

I also wonder if there should be an assertion/check checking snprintf’s
return value (as is done in the other places), but then again, it
doesn’t really matter.  But it would make the length limitation of
strncmp() superfluous and a plain strcmp() would suffice.

(I don’t like strncmp() very much here, because we have a problem if
indexstr’s length doesn’t suffice to hold children.%u, and we then
compare only the first INDEXSTR_LEN bytes with child->name.  That would
be a bug.  Just a different bug than an out-of-bounds read.)

> +        s->next_child_index--;
> +    }
> +
>      bdrv_drained_begin(bs);
> 
>      /* We can safely remove this child now */

Now, all in all, it doesn’t really matter of course.  This patch works
and introducing INDEXSTR_LEN isn’t worse than just using 32 as a magic
number.  So I’ll take the patch as-is for now – if you think that using
sizeof(indexstr) and/or replacing the strncmp() by assert()+strcmp()
would be so much better to justify a v4 (of this patch) or a follow-up
patch, then, well, that could still be done. O:)

tl;dr: Thanks, I’ve applied this patch to my block branch:

https://git.xanclic.moe/XanClic/qemu/commits/branch/block

Max