diff mbox

[v2,1/2] vvfat: Fix volume name assertion

Message ID 1461843366-27217-2-git-send-email-kwolf@redhat.com
State New, archived
Headers show

Commit Message

Kevin Wolf April 28, 2016, 11:36 a.m. UTC
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(-)

Comments

Peter Maydell April 28, 2016, 2:50 p.m. UTC | #1
On 28 April 2016 at 12:36, Kevin Wolf <kwolf@redhat.com> wrote:
> 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.

So you couldn't use this for writing at all, and we broke this
a year ago, and nobody complained til now? Shows how little
vvfat gets used...

thanks
-- PMM
Markus Armbruster April 28, 2016, 6:29 p.m. UTC | #2
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>
Stefan Hajnoczi April 29, 2016, 9:07 a.m. UTC | #3
On Thu, Apr 28, 2016 at 01:36:05PM +0200, Kevin Wolf wrote:
> 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(-)

I just noticed that Wolfgang's original patch got rid of the default
"QEMU VVFAT " volume label.  It now defaults to all spaces but I'm not
sure if this causes any problems...

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
diff mbox

Patch

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));
+
+        /* The first directory entry on the filesystem is the volume name */
+        first_direntry = (direntry_t*) s->directory.pointer;
+        assert(!memcmp(first_direntry->name, s->volume_label, 11));
+
 	current_dir_index += factor;
     }