Re: [PATCH 2/2] vvfat: Fix array_remove_slice()

[Eric Blake]

On 6/23/20 12:55 PM, Kevin Wolf wrote:
> array_remove_slice() calls array_roll() with array->next - 1 as the
> destination index. This is only correct for count == 1, otherwise we're
> writing past the end of the array. array->next - count would be correct.
>
> However, this is the only place ever calling array_roll(), so this
> rather complicated operation isn't even necessary.
>
> Fix the problem and simplify the code by replacing it with a single
> memmove() call. array_roll() can now be removed.
>
> Reported-by: Nathan Huckleberry <nhuck15@gmail.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block/vvfat.c | 42 +++++-------------------------------------
>   1 file changed, 5 insertions(+), 37 deletions(-)
>
> diff --git a/block/vvfat.c b/block/vvfat.c
> index 2fab371258..d6e464c595 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -140,48 +140,16 @@ static inline void* array_insert(array_t* array,unsigned 
> int index,unsigned int
>       return array->pointer+index*array->item_size;
>   }
>   
> -/* this performs a "roll", so that the element which was at index_from becomes
> - * index_to, but the order of all other elements is preserved. */
> -static inline int array_roll(array_t* array,int index_to,int index_from,int 
> count)

If I understand the intent from just the comment, the old code would 
take a directory listing of six files:

ABCDEF

and on the request to delete file C, would produce:

ABFDE

by moving just F, instead of all of DEF.  That might be what legacy FAT 
filesystems do natively, but I don't see it as a mandatory correctness 
issue; ABDEF is still a directory with the same contents.  And the bug 
for reading beyond array bounds when deleting more than one file is 
indeed nasty, so your simpler code is fine.

>   static inline int array_remove_slice(array_t* array,int index, int count)
>   {
>       assert(index >=0);
>       assert(count > 0);
>       assert(index + count <= array->next);
> -    if(array_roll(array,array->next-1,index,count))
> -        return -1;
> +
> +    memmove(array->pointer + index * array->item_size,
> +            array->pointer + (index + count) * array->item_size,
> +            (array->next - index - count) * array->item_size);
> +

Reviewed-by: Eric Blake <eblake@redhat.com>

>       array->next -= count;
>       return 0;
>   }

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org