Re: [PATCH] mv: prevent mismatched data when ignoring errors.
Michael Haggerty mhag...@alum.mit.edu 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.
Junio C Hamano gits...@pobox.com 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.
On 03/17/2014 07:33 AM, Junio C Hamano wrote:
Junio C Hamano gits...@pobox.com 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.
On Mon, Mar 17, 2014 at 11:07 AM, Michael Haggerty mhag...@alum.mit.edu wrote:
On 03/17/2014 07:33 AM, Junio C Hamano wrote:
Junio C Hamano gits...@pobox.com 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 Mon, Mar 17, 2014 at 03:06:02PM -0400, Eric Sunshine wrote:
On Mon, Mar 17, 2014 at 11:07 AM, Michael Haggerty mhag...@alum.mit.edu
wrote:
On 03/17/2014 07:33 AM, Junio C Hamano wrote:
Junio C Hamano gits...@pobox.com 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.
Jeff King p...@peff.net 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.
brian m. carlson sand...@crustytoothpaste.net 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 sand...@crustytoothpaste.net --- 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 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.
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 sand...@crustytoothpaste.net 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 sand...@crustytoothpaste.net --- 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 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 sand...@crustytoothpaste.net --- 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
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 sand...@crustytoothpaste.net --- 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
[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 sand...@crustytoothpaste.net --- 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
