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)
-{
-    char* buf;
-    char* from;
-    char* to;
-    int is;
-
-    if(!array ||
-            index_to<0 || index_to>=array->next ||
-            index_from<0 || index_from>=array->next)
-        return -1;
-
-    if(index_to==index_from)
-        return 0;
-
-    is=array->item_size;
-    from=array->pointer+index_from*is;
-    to=array->pointer+index_to*is;
-    buf=g_malloc(is*count);
-    memcpy(buf,from,is*count);
-
-    if(index_to<index_from)
-        memmove(to+is*count,to,from-to);
-    else
-        memmove(from,from+is*count,to-from);
-
-    memcpy(to,buf,is*count);
-
-    g_free(buf);
-
-    return 0;
-}
-
 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);
+
     array->next -= count;
     return 0;
 }
-- 
2.25.4

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

Am 23.06.2020 um 20:30 hat Eric Blake geschrieben:
> 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.

I think what the old code did was actually moving C, not F, so you get:

ABDEFC

And then you reduce the array size by one so that C isn't visible any
more. My code preserves this behaviour, except that the invisible final
element is a copy of F instead C now.

Kevin