Re: [PATCH] mv: prevent mismatched data when ignoring errors.
Michael Haggerty writes:
> I had recently been thinking along the same lines. In many of the
> potential callers that I noticed, ALLOC_GROW() was used immediately
> before making space in the array for a new element. So I suggest
> something more like
>
> +#define MOVE_DOWN(array, nr, at, count) \
> + memmove((array) + (at) + (count), \
> + (array) + (at), \
> + sizeof((array)[0]) * ((nr) - (at)))
> +#define ALLOC_INSERT_GAP(array, nr, at, count, alloc) \
> + do { \
> + ALLOC_GROW((array), (nr) + (count), (alloc));\
> + MOVE_DOWN((array), (nr), (at), (count)); \
> + } while (0)
>
> Also, count==1 is so frequent that this special case might deserve its
> own macro pair.
Yeah, probably.
> I'm not inspired by these macro names, though.
Me neither, about ups and downs.
Peff's suggestion to name these around the concept of "gap" sounded
sensible.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mv: prevent mismatched data when ignoring errors.
On Mon, Mar 17, 2014 at 03:06:02PM -0400, Eric Sunshine wrote:
> On Mon, Mar 17, 2014 at 11:07 AM, Michael Haggerty
> wrote:
> > On 03/17/2014 07:33 AM, Junio C Hamano wrote:
> >> Junio C Hamano writes:
> >>
> >>> Would it make sense to go one step further to introduce two macros
> >>> to make this kind of screw-up less likely?
> > potential callers that I noticed, ALLOC_GROW() was used immediately
> > before making space in the array for a new element. So I suggest
> > something more like
> >
> > +#define MOVE_DOWN(array, nr, at, count)\
> > + memmove((array) + (at) + (count), \
> > + (array) + (at), \
> > + sizeof((array)[0]) * ((nr) - (at)))
>
> Each time I read these, my brain (for whatever reason) interprets the
> names UP and DOWN opposite of the intended meaning, which makes them
> confusing. Perhaps INSERT_GAP and CLOSE_GAP would avoid such problems,
> and be more consistent with Michael's proposed ALLOC_INSERT_GAP.
Yeah, the UP/DOWN are very confusing to me. Something like SHRINK/EXPAND
(with the latter handling the ALLOC_GROW for us) makes more sense to me.
Those terms do not explicitly specify that we are doing it in the middle
(whereas GAP does), but I think it is fairly obvious from the parameters
what each is used for.
Side note: I had _almost_ added something to my original email
suggesting more use of macros to embody common idioms. For example, in
the vast majority of malloc cases, you could using something like:
#define ALLOC_OBJS(x,n) do { x = xmalloc(sizeof(*x) * (n)); } while(0)
#define ALLOC_OBJ(x) ALLOC_OBJS(x,1)
That eliminates a whole possible class of errors. But it's also
un-idiomatic as hell, and the resulting confusion can cause its own
problems. So I refrained from suggesting it.
I think as long as a macro is expressing a more high-level intent,
though, paying that cost can be worth it. By itself, wrapping memmove
to use sizeof(*array) does not seem all that exciting. But wrapping a
few specific cases like shrink/expand probably does make the code more
readable.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mv: prevent mismatched data when ignoring errors.
On Mon, Mar 17, 2014 at 11:07 AM, Michael Haggerty wrote:
> On 03/17/2014 07:33 AM, Junio C Hamano wrote:
>> Junio C Hamano writes:
>>
>>> Would it make sense to go one step further to introduce two macros
>>> to make this kind of screw-up less likely?
> potential callers that I noticed, ALLOC_GROW() was used immediately
> before making space in the array for a new element. So I suggest
> something more like
>
> +#define MOVE_DOWN(array, nr, at, count)\
> + memmove((array) + (at) + (count), \
> + (array) + (at), \
> + sizeof((array)[0]) * ((nr) - (at)))
Each time I read these, my brain (for whatever reason) interprets the
names UP and DOWN opposite of the intended meaning, which makes them
confusing. Perhaps INSERT_GAP and CLOSE_GAP would avoid such problems,
and be more consistent with Michael's proposed ALLOC_INSERT_GAP.
> +#define ALLOC_INSERT_GAP(array, nr, at, count, alloc) \
> + do { \
> + ALLOC_GROW((array), (nr) + (count), (alloc));\
> + MOVE_DOWN((array), (nr), (at), (count)); \
> + } while (0)
>
> Also, count==1 is so frequent that this special case might deserve its
> own macro pair.
>
> I'm not inspired by these macro names, though.
>
> Michael
>
> --
> Michael Haggerty
> mhag...@alum.mit.edu
> http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mv: prevent mismatched data when ignoring errors.
On 03/17/2014 07:33 AM, Junio C Hamano wrote:
> Junio C Hamano writes:
>
>> Would it make sense to go one step further to introduce two macros
>> to make this kind of screw-up less likely?
>> ...
>> 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.
>
> I see quite a many hits that follow this pattern
>
> memmove(array + pos, array + pos + 1, sizeof(*array) * (nr - pos))
>
> to make a single slot in a middle of array available, which would be
> good candidates to use MOVE_DOWN(). Just to show a few:
>
> builtin/mv.c:226: memmove(source + i, source + i + 1,
> builtin/mv.c-227- (argc - i) * sizeof(char *));
> builtin/mv.c:228: memmove(destination + i,
> builtin/mv.c-229- destination + i + 1,
> builtin/mv.c-230- (argc - i) * sizeof(char *));
> cache-tree.c:92: memmove(it->down + pos + 1,
> cache-tree.c-93- it->down + pos,
> cache-tree.c-94- sizeof(down) * (it->subtree_nr - pos - 1));
>
>
> Perhaps something like this patch to start off; I am not sure
> MOVE_DOWN_BOUNDED is needed, though.
>
> cache.h | 33 +
> 1 file changed, 33 insertions(+)
>
> diff --git a/cache.h b/cache.h
> index b66cb49..b2615ab 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -455,6 +455,39 @@ extern int daemonize(void);
> } \
> } while (0)
>
> +/*
> + * With an array "array" that currently holds "nr" elements, move
> + * elements at "at" and later down by "count" elements to make room to
> + * add in new elements. The caller is responsible for making sure
> + * that the array has enough room to hold "nr" + "count" slots.
> + */
> +#define MOVE_DOWN(array, nr, at, count) \
> + memmove((array) + (at) + (count), \
> + (array) + (at), \
> + sizeof((array)[0]) * ((nr) - (at)))
> +
> +/*
> + * With an array "array" that has enough memory to hold "alloc"
> + * elements allocated and currently holds "nr" elements, move elements
> + * at "at" and later down by "count" elements to make room to add in
> + * new elements.
> + */
> +#define MOVE_DOWN_BOUNDED(array, nr, at, count, alloc)\
> + do { \
> + if ((alloc) <= (nr) + (count)) \
> + BUG("MOVE_DOWN beyond the end of an array"); \
> + MOVE_DOWN((array), (nr), (at), (count)); \
> + } while (0)
> +
> +/*
> + * With an array "array" that curently holds "nr" elements, move elements
> + * at "at" + "count" and later down by "count" elements, removing the
> + * elements between "at" and "at" + "count".
> + */
> +#define MOVE_UP(array, nr, at, count)\
> + memmove((array) + (at), (array) + (at) + (count), \
> + sizeof((array)[0]) * ((nr) - ((at) + (count
> +
> /* Initialize and use the cache information */
> extern int read_index(struct index_state *);
> extern int read_index_preload(struct index_state *, const struct pathspec
> *pathspec);
I had recently been thinking along the same lines. In many of the
potential callers that I noticed, ALLOC_GROW() was used immediately
before making space in the array for a new element. So I suggest
something more like
+#define MOVE_DOWN(array, nr, at, count)\
+ memmove((array) + (at) + (count), \
+ (array) + (at), \
+ sizeof((array)[0]) * ((nr) - (at)))
+#define ALLOC_INSERT_GAP(array, nr, at, count, alloc) \
+ do { \
+ ALLOC_GROW((array), (nr) + (count), (alloc));\
+ MOVE_DOWN((array), (nr), (at), (count)); \
+ } while (0)
Also, count==1 is so frequent that this special case might deserve its
own macro pair.
I'm not inspired by these macro names, though.
Michael
--
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mv: prevent mismatched data when ignoring errors.
Junio C Hamano writes:
> Would it make sense to go one step further to introduce two macros
> to make this kind of screw-up less likely?
> ...
> 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.
I see quite a many hits that follow this pattern
memmove(array + pos, array + pos + 1, sizeof(*array) * (nr - pos))
to make a single slot in a middle of array available, which would be
good candidates to use MOVE_DOWN(). Just to show a few:
builtin/mv.c:226: memmove(source + i, source + i + 1,
builtin/mv.c-227- (argc - i) * sizeof(char *));
builtin/mv.c:228: memmove(destination + i,
builtin/mv.c-229- destination + i + 1,
builtin/mv.c-230- (argc - i) * sizeof(char *));
cache-tree.c:92:memmove(it->down + pos + 1,
cache-tree.c-93-it->down + pos,
cache-tree.c-94-sizeof(down) * (it->subtree_nr - pos - 1));
Perhaps something like this patch to start off; I am not sure
MOVE_DOWN_BOUNDED is needed, though.
cache.h | 33 +
1 file changed, 33 insertions(+)
diff --git a/cache.h b/cache.h
index b66cb49..b2615ab 100644
--- a/cache.h
+++ b/cache.h
@@ -455,6 +455,39 @@ extern int daemonize(void);
} \
} while (0)
+/*
+ * With an array "array" that currently holds "nr" elements, move
+ * elements at "at" and later down by "count" elements to make room to
+ * add in new elements. The caller is responsible for making sure
+ * that the array has enough room to hold "nr" + "count" slots.
+ */
+#define MOVE_DOWN(array, nr, at, count)\
+ memmove((array) + (at) + (count), \
+ (array) + (at), \
+ sizeof((array)[0]) * ((nr) - (at)))
+
+/*
+ * With an array "array" that has enough memory to hold "alloc"
+ * elements allocated and currently holds "nr" elements, move elements
+ * at "at" and later down by "count" elements to make room to add in
+ * new elements.
+ */
+#define MOVE_DOWN_BOUNDED(array, nr, at, count, alloc) \
+ do { \
+ if ((alloc) <= (nr) + (count)) \
+ BUG("MOVE_DOWN beyond the end of an array"); \
+ MOVE_DOWN((array), (nr), (at), (count)); \
+ } while (0)
+
+/*
+ * With an array "array" that curently holds "nr" elements, move elements
+ * at "at" + "count" and later down by "count" elements, removing the
+ * elements between "at" and "at" + "count".
+ */
+#define MOVE_UP(array, nr, at, count) \
+ memmove((array) + (at), (array) + (at) + (count), \
+ sizeof((array)[0]) * ((nr) - ((at) + (count
+
/* Initialize and use the cache information */
extern int read_index(struct index_state *);
extern int read_index_preload(struct index_state *, const struct pathspec
*pathspec);
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mv: prevent mismatched data when ignoring errors.
Jeff King writes: > 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mv: prevent mismatched data when ignoring errors.
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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mv: prevent mismatched data when ignoring errors.
"brian m. carlson" writes: > We shrink the source and destination arrays, but not the modes or > submodule_gitfile arrays, resulting in potentially mismatched data. Shrink > all the arrays at the same time to prevent this. > > Signed-off-by: brian m. carlson > --- > builtin/mv.c | 5 + > 1 file changed, 5 insertions(+) > > 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.) -- Thomas Rast t...@thomasrast.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mv: prevent mismatched data when ignoring errors.
On Tue, Mar 11, 2014 at 02:45:59PM -0700, Junio C Hamano wrote: > Thanks. Neither this nor John's seems to describe the user-visible > way to trigger the symptom. Can we have tests for them? I'll try to get to writing some test today or tomorrow. I just noticed the bugginess by looking at the code, so I'll need to actually spend time reproducing the problem. -- brian m. carlson / brian with sandals: Houston, Texas, US +1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187 signature.asc Description: Digital signature
Re: [PATCH] mv: prevent mismatched data when ignoring errors.
"brian m. carlson" writes: > We shrink the source and destination arrays, but not the modes or > submodule_gitfile arrays, resulting in potentially mismatched data. Shrink > all the arrays at the same time to prevent this. > > Signed-off-by: brian m. carlson > --- > builtin/mv.c | 5 + > 1 file changed, 5 insertions(+) > > 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 *)); > + memmove(submodule_gitfile + i, > + submodule_gitfile + i + 1, > + (argc - i) * sizeof(char *)); > i--; > } > } else Thanks. Neither this nor John's seems to describe the user-visible way to trigger the symptom. Can we have tests for them? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mv: prevent mismatched data when ignoring errors.
On Mon, Mar 10, 2014 at 09:56:03PM -0400, Jeff King wrote: > On Sat, Mar 08, 2014 at 07:21:39PM +, brian m. carlson wrote: > > > We shrink the source and destination arrays, but not the modes or > > submodule_gitfile arrays, resulting in potentially mismatched data. Shrink > > all the arrays at the same time to prevent this. > > > > Signed-off-by: brian m. carlson > > --- > > builtin/mv.c | 5 + > > 1 file changed, 5 insertions(+) > > > > 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 *)); > > + memmove(submodule_gitfile + i, > > + submodule_gitfile + i + 1, > > + (argc - i) * sizeof(char *)); > > I haven't looked that closely, but would it be crazy to suggest that > these arrays all be squashed into one array-of-struct? It would be less > error prone and perhaps more readable. I was thinking of doing exactly that, but the way internal_copy_pathspec is written, I'd need to use offsetof to have it write to the right structure members. That's a bit more gross than I wanted, but I'll probably implement it at some point during the upcoming week. -- brian m. carlson / brian with sandals: Houston, Texas, US +1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187 signature.asc Description: Digital signature
Re: [PATCH] mv: prevent mismatched data when ignoring errors.
On Sat, Mar 08, 2014 at 07:21:39PM +, brian m. carlson wrote: > We shrink the source and destination arrays, but not the modes or > submodule_gitfile arrays, resulting in potentially mismatched data. Shrink > all the arrays at the same time to prevent this. > > Signed-off-by: brian m. carlson > --- > builtin/mv.c | 5 + > 1 file changed, 5 insertions(+) > > 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 *)); > + memmove(submodule_gitfile + i, > + submodule_gitfile + i + 1, > + (argc - i) * sizeof(char *)); I haven't looked that closely, but would it be crazy to suggest that these arrays all be squashed into one array-of-struct? It would be less error prone and perhaps more readable. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] mv: prevent mismatched data when ignoring errors.
We shrink the source and destination arrays, but not the modes or submodule_gitfile arrays, resulting in potentially mismatched data. Shrink all the arrays at the same time to prevent this. Signed-off-by: brian m. carlson --- builtin/mv.c | 5 + 1 file changed, 5 insertions(+) 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 *)); + memmove(submodule_gitfile + i, + submodule_gitfile + i + 1, + (argc - i) * sizeof(char *)); i--; } } else -- 1.9.0.1010.g6633b85.dirty -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
