Re: [Qemu-devel] [PATCH v2 1/2] vvfat: Fix volume name assertion

From: Markus Armbruster <armbru@redhat.com>
To: Kevin Wolf <kwolf@redhat.com>
Cc: qemu-block@nongnu.org, w.bumiller@proxmox.com,
	qemu-devel@nongnu.org, mreitz@redhat.com, stefanha@redhat.com
Subject: Re: [Qemu-devel] [PATCH v2 1/2] vvfat: Fix volume name assertion
Date: Thu, 28 Apr 2016 20:29:32 +0200	[thread overview]
Message-ID: <871t5pd1tf.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <1461843366-27217-2-git-send-email-kwolf@redhat.com> (Kevin Wolf's message of "Thu, 28 Apr 2016 13:36:05 +0200")

Kevin Wolf <kwolf@redhat.com> writes:

> Commit d5941dd made the volume name configurable, but it didn't consider
> that the rw code compares the volume name string to assert that the
> first directory entry is the volume name. This made vvfat crash in rw
> mode.
>
> This fixes the assertion to compare with the configured volume name
> instead of a literal string.
>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/vvfat.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/block/vvfat.c b/block/vvfat.c
> index 6b85314..ff3df35 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -2283,12 +2283,17 @@ DLOG(fprintf(stderr, "commit_direntries for %s, parent_mapping_index %d\n", mapp
>  		factor * (old_cluster_count - new_cluster_count));
>  
>      for (c = first_cluster; !fat_eof(s, c); c = modified_fat_get(s, c)) {
> +        direntry_t *first_direntry;
>  	void* direntry = array_get(&(s->directory), current_dir_index);
>  	int ret = vvfat_read(s->bs, cluster2sector(s, c), direntry,
>  		s->sectors_per_cluster);
>  	if (ret)
>  	    return ret;
> -	assert(!strncmp(s->directory.pointer, "QEMU", 4));

Typing all of "QEMU VVAT" a third time was clearly too much.

> +
> +        /* The first directory entry on the filesystem is the volume name */
> +        first_direntry = (direntry_t*) s->directory.pointer;

I'd ask to correct the spacing to (direntry_t *)s if the spacing wasn't
similarly off all over this file.

> +        assert(!memcmp(first_direntry->name, s->volume_label, 11));
> +
>  	current_dir_index += factor;
>      }

Might want to to assert is_volume_label(), too.  But even if you want
to, let's not delay the fix for that.

Reviewed-by: Markus Armbruster <armbru@redhat.com>