Re: [PATCH] mv: prevent mismatched data when ignoring errors.

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 16, 2014, 21:20 UTC
Message-ID: <7v1ty14z8x.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20140316020018.GA20019@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 22 quoted lines

> On Sat, Mar 15, 2014 at 05:05:29PM +0100, Thomas Rast wrote:
>
>> > diff --git a/builtin/mv.c b/builtin/mv.c
>> > index f99c91e..b20cd95 100644
>> > --- a/builtin/mv.c
>> > +++ b/builtin/mv.c
>> > @@ -230,6 +230,11 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>> >  					memmove(destination + i,
>> >  						destination + i + 1,
>> >  						(argc - i) * sizeof(char *));
>> > +					memmove(modes + i, modes + i + 1,
>> > +						(argc - i) * sizeof(char *));
>> 
>> This isn't right -- you are computing the size of things to be moved
>> based on a type of char*, but 'modes' is an enum.
>> 
>> (Valgrind spotted this.)
>
> Maybe using sizeof(*destination) and sizeof(*modes) would make this less
> error-prone?
>
> -Peff

Would it make sense to go one step further to introduce two macros to make this kind of screw-up less likely?

 1. "array" is an array that holds "nr" elements.  Move "count"
    elements starting at index "at" down to remove them.

    #define MOVE_DOWN(array, nr, at, count)

    The implementation should take advantage of sizeof(*array) to
    come up with the number of bytes to move.

 2. "array" is an array that holds "nr" elements.  Move "count"
    elements starting at index "at" up to make room to copy new
    elements in.

    #define MOVE_UP(array, nr, at, count)

    The implementation should take advantage of sizeof(*array) to
    come up with the number of bytes to move.

Optionally, to make 2. even safer, these macros could take "alloc" to say that "array" has memory allocated to hold "alloc" elements, and the implementation may check "nr + count" does not overflow "alloc". This would make 1. and 2. asymmetric (move-down can do no validation using "alloc", but move-up would be helped), so I am not sure it is a good idea.

After letting my eyes coast over hits from "git grep memmove", there do seem to be some places that these would help readability, but not very many.