patch, 3 partsfixes for running the test suite with --valgrind

> Avoid this by checking passing in the length of the string in the char
> array, and checking that we never run over it.
>
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> ---
>  path.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)

> When I first read the above, I thought it was going to be about a
> NUL-terminated string that was missing a NUL.  But in fact, the issue
> is that strlen(path) can be < 2.
> 
> In other words, an alternative fix would be
> 
> 	if (*path == '.' && path[1] == '/') {
> 		...
> 	}
> 
> which would not require passing in 'len' or switching to index-based
> arithmetic.  I think I prefer it.  What do you think?

>> In other words, an alternative fix would be
>> 
>> 	if (*path == '.' && path[1] == '/') {
>> 		...
>> 	}
>> 
>> which would not require passing in 'len' or switching to index-based
>> arithmetic.  I think I prefer it.  What do you think?
>
> Yes, I think that approach is much nicer. I think you could even use
> skip_prefix. Unfortunately you have to play a few games with const-ness,
> but I think the resulting signature for cleanup_path() is an
> improvement:

> diff --git a/path.c b/path.c
> index 00ec04e7a5..2e09a7bce0 100644
> --- a/path.c
> +++ b/path.c
> @@ -34,11 +34,10 @@ static struct strbuf *get_pathname(void)
>  	return sb;
>  }
>  
> -static char *cleanup_path(char *path)
> +static const char *cleanup_path(const char *path)
>  {
>  	/* Clean it up */
> -	if (!memcmp(path, "./", 2)) {
> -		path += 2;
> +	if (skip_prefix(path, "./", &path)) {
>  		while (*path == '/')
>  			path++;
>  	}
> @@ -47,7 +46,7 @@ static char *cleanup_path(char *path)
>  
>  static void strbuf_cleanup_path(struct strbuf *sb)
>  {
> -	char *path = cleanup_path(sb->buf);
> +	const char *path = cleanup_path(sb->buf);
>  	if (path > sb->buf)
>  		strbuf_remove(sb, 0, path - sb->buf);
>  }
> @@ -64,7 +63,7 @@ char *mksnpath(char *buf, size_t n, const char *fmt, ...)
>  		strlcpy(buf, bad_path, n);
>  		return buf;
>  	}
> -	return cleanup_path(buf);
> +	return (char *)cleanup_path(buf);
>  }
>  
>  static int dir_prefix(const char *buf, const char *dir)

> Jeff King wrote:
>> On Tue, Oct 03, 2017 at 03:45:01PM -0700, Jonathan Nieder wrote:
>
>>> In other words, an alternative fix would be
>>> 
>>> 	if (*path == '.' && path[1] == '/') {
>>> 		...
>>> 	}
>>> 
>>> which would not require passing in 'len' or switching to index-based
>>> arithmetic.  I think I prefer it.  What do you think?
>>
>> Yes, I think that approach is much nicer. I think you could even use
>> skip_prefix. Unfortunately you have to play a few games with const-ness,
>> but I think the resulting signature for cleanup_path() is an
>> improvement:

> Jonathan Nieder <jrnieder@gmail.com> writes:
> 
> > Jeff King wrote:
> >> On Tue, Oct 03, 2017 at 03:45:01PM -0700, Jonathan Nieder wrote:
> >
> >>> In other words, an alternative fix would be
> >>> 
> >>> 	if (*path == '.' && path[1] == '/') {
> >>> 		...
> >>> 	}
> >>> 
> >>> which would not require passing in 'len' or switching to index-based
> >>> arithmetic.  I think I prefer it.  What do you think?
> >>
> >> Yes, I think that approach is much nicer. I think you could even use
> >> skip_prefix. Unfortunately you have to play a few games with const-ness,
> >> but I think the resulting signature for cleanup_path() is an
> >> improvement:
> 
> To tie the loose end, here is what I'll queue.

> Jonathan Nieder <jrnieder@gmail.com> writes:
> 
> > Jeff King wrote:
> >> On Tue, Oct 03, 2017 at 03:45:01PM -0700, Jonathan Nieder wrote:
> >
> >>> In other words, an alternative fix would be
> >>> 
> >>> 	if (*path == '.' && path[1] == '/') {
> >>> 		...
> >>> 	}
> >>> 
> >>> which would not require passing in 'len' or switching to index-based
> >>> arithmetic.  I think I prefer it.  What do you think?
> >>
> >> Yes, I think that approach is much nicer. I think you could even use
> >> skip_prefix. Unfortunately you have to play a few games with const-ness,
> >> but I think the resulting signature for cleanup_path() is an
> >> improvement:
> 
> To tie the loose end, here is what I'll queue.

> -- >8 --
> From: Jeff King <peff@peff.net>
> Date: Tue, 3 Oct 2017 19:30:40 -0400
> Subject: [PATCH] path.c: fix uninitialized memory access
> 
> In cleanup_path we're passing in a char array, run a memcmp on it, and
> run through it without ever checking if something is in the array in the
> first place.  This can lead us to access uninitialized memory, for
> example in t5541-http-push-smart.sh test 7, when run under valgrind:
> 
> ==4423== Conditional jump or move depends on uninitialised value(s)
> ==4423==    at 0x242FA9: cleanup_path (path.c:35)
> ==4423==    by 0x242FA9: mkpath (path.c:456)
> ==4423==    by 0x256CC7: refname_match (refs.c:364)
> ==4423==    by 0x26C181: count_refspec_match (remote.c:1015)
> ==4423==    by 0x26C181: match_explicit_lhs (remote.c:1126)
> ==4423==    by 0x26C181: check_push_refs (remote.c:1409)
> ==4423==    by 0x2ABB4D: transport_push (transport.c:870)
> ==4423==    by 0x186703: push_with_options (push.c:332)
> ==4423==    by 0x18746D: do_push (push.c:409)
> ==4423==    by 0x18746D: cmd_push (push.c:566)
> ==4423==    by 0x1183E0: run_builtin (git.c:352)
> ==4423==    by 0x11973E: handle_builtin (git.c:539)
> ==4423==    by 0x11973E: run_argv (git.c:593)
> ==4423==    by 0x11973E: main (git.c:698)
> ==4423==  Uninitialised value was created by a heap allocation
> ==4423==    at 0x4C2CD8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==4423==    by 0x4C2F195: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==4423==    by 0x2C196B: xrealloc (wrapper.c:137)
> ==4423==    by 0x29A30B: strbuf_grow (strbuf.c:66)
> ==4423==    by 0x29A30B: strbuf_vaddf (strbuf.c:277)
> ==4423==    by 0x242F9F: mkpath (path.c:454)
> ==4423==    by 0x256CC7: refname_match (refs.c:364)
> ==4423==    by 0x26C181: count_refspec_match (remote.c:1015)
> ==4423==    by 0x26C181: match_explicit_lhs (remote.c:1126)
> ==4423==    by 0x26C181: check_push_refs (remote.c:1409)
> ==4423==    by 0x2ABB4D: transport_push (transport.c:870)
> ==4423==    by 0x186703: push_with_options (push.c:332)
> ==4423==    by 0x18746D: do_push (push.c:409)
> ==4423==    by 0x18746D: cmd_push (push.c:566)
> ==4423==    by 0x1183E0: run_builtin (git.c:352)
> ==4423==    by 0x11973E: handle_builtin (git.c:539)
> ==4423==    by 0x11973E: run_argv (git.c:593)
> ==4423==    by 0x11973E: main (git.c:698)
> ==4423==
> 
> Avoid this by using skip_prefix(), which knows not to go beyond the
> end of the string.
> 
> Reported-by: Thomas Gummerer <t.gummerer@gmail.com>
> Signed-off-by: Jeff King <peff@peff.net>
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  path.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/path.c b/path.c
> index e50d2befcf..2fecf854fe 100644
> --- a/path.c
> +++ b/path.c
> @@ -33,11 +33,10 @@ static struct strbuf *get_pathname(void)
>  	return sb;
>  }
>  
> -static char *cleanup_path(char *path)
> +static const char *cleanup_path(const char *path)
>  {
>  	/* Clean it up */
> -	if (!memcmp(path, "./", 2)) {
> -		path += 2;
> +	if (skip_prefix(path, "./", &path)) {
>  		while (*path == '/')
>  			path++;
>  	}
> @@ -46,7 +45,7 @@ static char *cleanup_path(char *path)
>  
>  static void strbuf_cleanup_path(struct strbuf *sb)
>  {
> -	char *path = cleanup_path(sb->buf);
> +	const char *path = cleanup_path(sb->buf);
>  	if (path > sb->buf)
>  		strbuf_remove(sb, 0, path - sb->buf);
>  }
> @@ -63,7 +62,7 @@ char *mksnpath(char *buf, size_t n, const char *fmt, ...)
>  		strlcpy(buf, bad_path, n);
>  		return buf;
>  	}
> -	return cleanup_path(buf);
> +	return (char *)cleanup_path(buf);
>  }
>  
>  static int dir_prefix(const char *buf, const char *dir)
> -- 
> 2.14.2-889-gd2948f6aa6
> 

> From: Jeff King <peff@peff.net>
> Date: Tue, 3 Oct 2017 19:30:40 -0400
> Subject: [PATCH] path.c: fix uninitialized memory access
> 
> In cleanup_path we're passing in a char array, run a memcmp on it, and
> run through it without ever checking if something is in the array in the
> first place.  This can lead us to access uninitialized memory, for
> example in t5541-http-push-smart.sh test 7, when run under valgrind:
>
> ==4423== Conditional jump or move depends on uninitialised value(s)
> ==4423==    at 0x242FA9: cleanup_path (path.c:35)

> Avoid this by using skip_prefix(), which knows not to go beyond the
> end of the string.
>
> Reported-by: Thomas Gummerer <t.gummerer@gmail.com>
> Signed-off-by: Jeff King <peff@peff.net>
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

> The get_oid_hex_from_objpath takes care of creating a oid from a
> pathname.  It does this by memcpy'ing the first two bytes of the path to
> the "hex" string, then skipping the '/', and then copying the rest of the
> path to the "hex" string.  Currently it fails to increase the pointer to
> the hex string, so the second memcpy invocation just mashes over what
> was copied in the first one, and leaves the last two bytes in the string
> uninitialized.

> diff --git a/http-push.c b/http-push.c
> index e4c9b065ce..e9a01ec4da 100644
> --- a/http-push.c
> +++ b/http-push.c
> @@ -1018,7 +1018,7 @@ static int get_oid_hex_from_objpath(const char *path, struct object_id *oid)
>  	memcpy(hex, path, 2);
>  	path += 2;
>  	path++; /* skip '/' */
> -	memcpy(hex, path, GIT_SHA1_HEXSZ - 2);
> +	memcpy(hex + 2, path, GIT_SHA1_HEXSZ - 2);
>  
>  	return get_oid_hex(hex, oid);

> Thomas Gummerer wrote:
> 
> > The get_oid_hex_from_objpath takes care of creating a oid from a
> > pathname.  It does this by memcpy'ing the first two bytes of the path to
> > the "hex" string, then skipping the '/', and then copying the rest of the
> > path to the "hex" string.  Currently it fails to increase the pointer to
> > the hex string, so the second memcpy invocation just mashes over what
> > was copied in the first one, and leaves the last two bytes in the string
> > uninitialized.
> 
> Wow.  The fix is obviously correct.

> I think the problem is that when it fails, we end up thinking that
> there are *fewer* objects than are actually present remotely so the
> only ill effect is pushing too much.  So this should be observable in
> server logs (i.e. it is testable) but it's not a catastrophic failure
> which means it's harder to test than it would be otherwise.

>> Moreover, this is in the webdav-based "dumb http" push code path,
>> which I do not trust much at all.  I wonder if we could retire it
>> completely (or at least provide an option to turn it off).
>
> I would really like that, too. It has been the cause of a lot of pain
> when working with the smart code, and I am not at all surprised to find
> a bug of this magnitude lurking in it. I'd _hoped_ this could show that
> the system has been unusably broken for years, which would give us
> confidence to turn it off. :) But per your paragraph above, people could
> very easily still have been happily using it in the meantime.

> Jeff King <peff@peff.net> writes:
>
>>> Moreover, this is in the webdav-based "dumb http" push code path,
>>> which I do not trust much at all.  I wonder if we could retire it
>>> completely (or at least provide an option to turn it off).
>>
>> I would really like that, too. It has been the cause of a lot of pain
>> when working with the smart code, and I am not at all surprised to find
>> a bug of this magnitude lurking in it. I'd _hoped_ this could show that
>> the system has been unusably broken for years, which would give us
>> confidence to turn it off. :) But per your paragraph above, people could
>> very easily still have been happily using it in the meantime.
>
> Same here.  Perhaps we should deliberately and silently break it and
> see who screams?

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Jeff King <peff@peff.net> writes:
> >
> >>> Moreover, this is in the webdav-based "dumb http" push code path,
> >>> which I do not trust much at all.  I wonder if we could retire it
> >>> completely (or at least provide an option to turn it off).
> >>
> >> I would really like that, too. It has been the cause of a lot of pain
> >> when working with the smart code, and I am not at all surprised to find
> >> a bug of this magnitude lurking in it. I'd _hoped_ this could show that
> >> the system has been unusably broken for years, which would give us
> >> confidence to turn it off. :) But per your paragraph above, people could
> >> very easily still have been happily using it in the meantime.
> >
> > Same here.  Perhaps we should deliberately and silently break it and
> > see who screams?
> 
> Hopefully it should be obvious but just for people with unreasonable
> expectations, I should clarify that the above needs a smiley ;-).

> diff --git a/sub-process.c b/sub-process.c
> index 6dde5062be..4680af8193 100644
> --- a/sub-process.c
> +++ b/sub-process.c
> @@ -77,7 +77,9 @@ int subprocess_start(struct hashmap *hashmap, struct subprocess_entry *entry, co
>   {
>   	int err;
>   	struct child_process *process;
> -	const char *argv[] = { cmd, NULL };
> +	const char **argv = xmalloc(2 * sizeof(char *));
> +	argv[0] = cmd;
> +	argv[1] = NULL;
>   
>   	entry->cmd = cmd;
>   	process = &entry->process;
> 

> Am 03.10.2017 um 21:57 schrieb Thomas Gummerer:
>> diff --git a/sub-process.c b/sub-process.c
>> index 6dde5062be..4680af8193 100644
>> --- a/sub-process.c
>> +++ b/sub-process.c
>> @@ -77,7 +77,9 @@ int subprocess_start(struct hashmap *hashmap, struct subprocess_entry *entry, co
>>   {
>>   	int err;
>>   	struct child_process *process;
>> -	const char *argv[] = { cmd, NULL };
>> +	const char **argv = xmalloc(2 * sizeof(char *));
>> +	argv[0] = cmd;
>> +	argv[1] = NULL;
>>     	entry->cmd = cmd;
>>   	process = &entry->process;
>>
>
> Perhaps this should become
>
> 	argv_array_push(&process->args, cmd);
>
> so that there is no new memory leak?

> > Perhaps this should become
> >
> > 	argv_array_push(&process->args, cmd);
> >
> > so that there is no new memory leak?
> 
> Sounds like a good idea (if I am not grossly mistaken as to what is
> being suggested).
> 
> Here is what I am planning to queue.
> 
> -- >8 --
> From: Johannes Sixt <j6t@kdbg.org>
> Date: Tue, 3 Oct 2017 22:24:57 +0200
> Subject: [PATCH] sub-process: use child_process.args instead of child_process.argv

> Johannes Sixt <j6t@kdbg.org> writes:
> 
>> Am 03.10.2017 um 21:57 schrieb Thomas Gummerer:
>>> diff --git a/sub-process.c b/sub-process.c
>>> index 6dde5062be..4680af8193 100644
>>> --- a/sub-process.c
>>> +++ b/sub-process.c
>>> @@ -77,7 +77,9 @@ int subprocess_start(struct hashmap *hashmap, struct subprocess_entry *entry, co
>>>    {
>>>    	int err;
>>>    	struct child_process *process;
>>> -	const char *argv[] = { cmd, NULL };
>>> +	const char **argv = xmalloc(2 * sizeof(char *));
>>> +	argv[0] = cmd;
>>> +	argv[1] = NULL;
>>>      	entry->cmd = cmd;
>>>    	process = &entry->process;
>>>
>>
>> Perhaps this should become
>>
>> 	argv_array_push(&process->args, cmd);
>>
>> so that there is no new memory leak?
> 
> Sounds like a good idea (if I am not grossly mistaken as to what is
> being suggested).
> 
> Here is what I am planning to queue.
> 
> -- >8 --
> From: Johannes Sixt <j6t@kdbg.org>
> Date: Tue, 3 Oct 2017 22:24:57 +0200
> Subject: [PATCH] sub-process: use child_process.args instead of child_process.argv
> 
> Currently the argv is only allocated on the stack, and then assigned to
> process->argv.  When the start_subprocess function goes out of scope,
> the local argv variable is eliminated from the stack, but the pointer is
> still kept around in process->argv.
> 
> Much later when we try to access the same process->argv in
> finish_command, this leads us to access a memory location that no longer
> contains what we want.  As argv0 is only used for printing errors, this
> is not easily noticed in normal git operations.  However when running
> t0021-conversion.sh through valgrind, valgrind rightfully complains:
> 
> ==21024== Invalid read of size 8
> ==21024==    at 0x2ACF64: finish_command (run-command.c:869)
> ==21024==    by 0x2D6B18: subprocess_exit_handler (sub-process.c:72)
> ==21024==    by 0x2AB41E: cleanup_children (run-command.c:45)
> ==21024==    by 0x2AB526: cleanup_children_on_exit (run-command.c:81)
> ==21024==    by 0x54AD487: __run_exit_handlers (in /usr/lib/libc-2.26.so)
> ==21024==    by 0x54AD4D9: exit (in /usr/lib/libc-2.26.so)
> ==21024==    by 0x11A9EF: handle_builtin (git.c:550)
> ==21024==    by 0x11ABCC: run_argv (git.c:602)
> ==21024==    by 0x11AD8E: cmd_main (git.c:679)
> ==21024==    by 0x1BF125: main (common-main.c:43)
> ==21024==  Address 0x1ffeffec00 is on thread 1's stack
> ==21024==  1504 bytes below stack pointer
> ==21024==
> 
> These days, the child_process structure has its own args array, and
> the standard way to set up its argv[] is to use that one, instead of
> assigning to process->argv to point at an array that is outside.
> Use that facility automatically fixes this issue.
> 
> Reported-by: Thomas Gummerer <t.gummerer@gmail.com>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>   sub-process.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/sub-process.c b/sub-process.c
> index fcc4832c14..648b3a3943 100644
> --- a/sub-process.c
> +++ b/sub-process.c
> @@ -74,13 +74,12 @@ int subprocess_start(struct hashmap *hashmap, struct subprocess_entry *entry, co
>   {
>   	int err;
>   	struct child_process *process;
> -	const char *argv[] = { cmd, NULL };
>   
>   	entry->cmd = cmd;
>   	process = &entry->process;
>   
>   	child_process_init(process);
> -	process->argv = argv;
> +	argv_array_push(&process->args, cmd);
>   	process->use_shell = 1;
>   	process->in = -1;
>   	process->out = -1;
> 

> Johannes Sixt <j6t@kdbg.org> writes:
> 
> > Am 03.10.2017 um 21:57 schrieb Thomas Gummerer:
> >> diff --git a/sub-process.c b/sub-process.c
> >> index 6dde5062be..4680af8193 100644
> >> --- a/sub-process.c
> >> +++ b/sub-process.c
> >> @@ -77,7 +77,9 @@ int subprocess_start(struct hashmap *hashmap, struct subprocess_entry *entry, co
> >>   {
> >>   	int err;
> >>   	struct child_process *process;
> >> -	const char *argv[] = { cmd, NULL };
> >> +	const char **argv = xmalloc(2 * sizeof(char *));
> >> +	argv[0] = cmd;
> >> +	argv[1] = NULL;
> >>     	entry->cmd = cmd;
> >>   	process = &entry->process;
> >>
> >
> > Perhaps this should become
> >
> > 	argv_array_push(&process->args, cmd);
> >
> > so that there is no new memory leak?
> 
> Sounds like a good idea (if I am not grossly mistaken as to what is
> being suggested).
> 
> Here is what I am planning to queue.
> 
> -- >8 --
> From: Johannes Sixt <j6t@kdbg.org>
> Date: Tue, 3 Oct 2017 22:24:57 +0200
> Subject: [PATCH] sub-process: use child_process.args instead of child_process.argv
> 
> Currently the argv is only allocated on the stack, and then assigned to
> process->argv.  When the start_subprocess function goes out of scope,
> the local argv variable is eliminated from the stack, but the pointer is
> still kept around in process->argv.
> 
> Much later when we try to access the same process->argv in
> finish_command, this leads us to access a memory location that no longer
> contains what we want.  As argv0 is only used for printing errors, this
> is not easily noticed in normal git operations.  However when running
> t0021-conversion.sh through valgrind, valgrind rightfully complains:
> 
> ==21024== Invalid read of size 8
> ==21024==    at 0x2ACF64: finish_command (run-command.c:869)
> ==21024==    by 0x2D6B18: subprocess_exit_handler (sub-process.c:72)
> ==21024==    by 0x2AB41E: cleanup_children (run-command.c:45)
> ==21024==    by 0x2AB526: cleanup_children_on_exit (run-command.c:81)
> ==21024==    by 0x54AD487: __run_exit_handlers (in /usr/lib/libc-2.26.so)
> ==21024==    by 0x54AD4D9: exit (in /usr/lib/libc-2.26.so)
> ==21024==    by 0x11A9EF: handle_builtin (git.c:550)
> ==21024==    by 0x11ABCC: run_argv (git.c:602)
> ==21024==    by 0x11AD8E: cmd_main (git.c:679)
> ==21024==    by 0x1BF125: main (common-main.c:43)
> ==21024==  Address 0x1ffeffec00 is on thread 1's stack
> ==21024==  1504 bytes below stack pointer
> ==21024==
> 
> These days, the child_process structure has its own args array, and
> the standard way to set up its argv[] is to use that one, instead of
> assigning to process->argv to point at an array that is outside.
> Use that facility automatically fixes this issue.
> 
> Reported-by: Thomas Gummerer <t.gummerer@gmail.com>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  sub-process.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/sub-process.c b/sub-process.c
> index fcc4832c14..648b3a3943 100644
> --- a/sub-process.c
> +++ b/sub-process.c
> @@ -74,13 +74,12 @@ int subprocess_start(struct hashmap *hashmap, struct subprocess_entry *entry, co
>  {
>  	int err;
>  	struct child_process *process;
> -	const char *argv[] = { cmd, NULL };
>  
>  	entry->cmd = cmd;
>  	process = &entry->process;
>  
>  	child_process_init(process);
> -	process->argv = argv;
> +	argv_array_push(&process->args, cmd);

>  	process->use_shell = 1;
>  	process->in = -1;
>  	process->out = -1;
> -- 
> 2.14.2-889-gd2948f6aa6
> 

> Currently the argv is only allocated on the stack, and then assigned to
> process->argv.  When the start_subprocess function goes out of scope,
> the local argv variable is eliminated from the stack, but the pointer is
> still kept around in process->argv.
>
> Much later when we try to access the same process->argv in
> finish_command, this leads us to access a memory location that no longer
> contains what we want.  As argv0 is only used for printing errors, this
> is not easily noticed in normal git operations.  However when running
> t0021-conversion.sh through valgrind, valgrind rightfully complains:
>
> ==21024== Invalid read of size 8
> ==21024==    at 0x2ACF64: finish_command (run-command.c:869)
> ==21024==    by 0x2D6B18: subprocess_exit_handler (sub-process.c:72)
> ==21024==    by 0x2AB41E: cleanup_children (run-command.c:45)
> ==21024==    by 0x2AB526: cleanup_children_on_exit (run-command.c:81)
> ==21024==    by 0x54AD487: __run_exit_handlers (in /usr/lib/libc-2.26.so)
> ==21024==    by 0x54AD4D9: exit (in /usr/lib/libc-2.26.so)
> ==21024==    by 0x11A9EF: handle_builtin (git.c:550)
> ==21024==    by 0x11ABCC: run_argv (git.c:602)
> ==21024==    by 0x11AD8E: cmd_main (git.c:679)
> ==21024==    by 0x1BF125: main (common-main.c:43)
> ==21024==  Address 0x1ffeffec00 is on thread 1's stack
> ==21024==  1504 bytes below stack pointer
> ==21024==
>
> Fix this by allocating the memory on properly on the heap.  This memory
> is allocated on the heap, and never free'd.  However the same seems to be
> true for struct child_process, so it should be fine to just let the
> memory be free'd when the process terminates.

> > I think using SANITIZE=memory would catch these, but it needs some
> > suppressions tuning. The weird "zlib reads uninitialized memory" error
> > is a problem (valgrind sees this, too, but we have suppressions).
> 
> What version of zlib do you use?  I've heard some good things about
> v1.2.11 improving matters, though I haven't checked yet.

> On Tue, Oct 03, 2017 at 07:41:54PM -0400, Jeff King wrote:
> 
> > I think using SANITIZE=memory would catch these, but it needs some
> > suppressions tuning. The weird "zlib reads uninitialized memory" error
> > is a problem (valgrind sees this, too, but we have suppressions).
> 
> I dug into this a little more. You can blacklist certain functions from
> getting MSan treatment, but that's not quite what we want. We want to
> mark bytes from certain _sources_ as being initialized, even if MSan
> doesn't agree.
> 
> And indeed, you can do that. As far as I can tell, MSan works by keeping
> a shadow map of memory and setting flags when it believes it has been
> initialized, and then checking that map when we make decisions based on
> the memory. But it can only do that if it instruments all writes. So the
> MSan documentation recommends that you build _everything_, including
> libraries, with it. Which obviously we don't do if we're using a system
> zlib. Or a system libc for that matter (though they intercept many
> common libc functions to handle this).
> 
> So one strategy is to "cheat" a bit at the library interfaces, and claim
> whatever they send us is properly initialized. The patch below tries
> that with zlib, and it does seem to work. It would fail to notice a real
> problem with any input we send _to_ the library (since the library isn't
> instrumented, and we claim that whatever comes out of it is legitimate).
> I could probably live with that.
> 
> But there are quite a few test failures that would still need
> investigating and annotating:
> 
>   - Certainly it's confused by looking at regmatch_t results from
>     regexec(). We can fix that by building with NO_REGEX. But pcre has
>     a similar problem.
> 
>   - Ditto curl and openssl, whose exit points would need annotations.
> 
>   - For some reason test-sigchain segfaults when it raise()s in the
>     signal handler and recurses. Not sure if this is an MSan bug or
>     what.
> 
> So I dunno. This approach is a _lot_ more convenient than trying to
> rebuild all the dependencies from scratch, and it runs way faster than
> valgrind.  It did find the cases that led to the patches in this
> series, and at least one more: if the lstat() at the end of
> entry.c:write_entry() fails, we write nonsense into the cache_entry.

> I think we could probably get it to zero false positives without _too_
> much effort. I'll stop here for tonight, but I may pick it up again
> later (of course anybody else is welcome to fool around with it, too).
> 
> Below is the patch that let me run:
> 
>   make SANITIZE=memory CC=clang-6.0 NO_REGEX=1
> 
> and get a tractable number of errors.
> 
> -- >8 --
> diff --git a/Makefile b/Makefile
> index b143e4eea3..1da5c01211 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1047,6 +1047,9 @@ endif
>  ifneq ($(filter leak,$(SANITIZERS)),)
>  BASIC_CFLAGS += -DSUPPRESS_ANNOTATED_LEAKS
>  endif
> +ifneq ($(filter memory,$(SANITIZERS)),)
> +BASIC_CFLAGS += -DENABLE_MSAN_UNPOISON
> +endif
>  endif
>  
>  ifndef sysconfdir
> diff --git a/git-compat-util.h b/git-compat-util.h
> index cedad4d581..836a4c0b54 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -1191,4 +1191,11 @@ extern void unleak_memory(const void *ptr, size_t len);
>  #define UNLEAK(var) do {} while (0)
>  #endif
>  
> +#ifdef ENABLE_MSAN_UNPOISON
> +#include <sanitizer/msan_interface.h>
> +#define msan_unpoison(ptr, len) __msan_unpoison(ptr, len)
> +#else
> +#define msan_unpoison(ptr, len) do {} while (0)
> +#endif
> +
>  #endif
> diff --git a/zlib.c b/zlib.c
> index 4223f1a8c5..5fa8f12507 100644
> --- a/zlib.c
> +++ b/zlib.c
> @@ -56,6 +56,8 @@ static void zlib_post_call(git_zstream *s)
>  	if (s->z.total_in != s->total_in + bytes_consumed)
>  		die("BUG: total_in mismatch");
>  
> +	msan_unpoison(s->next_out, bytes_produced);
> +
>  	s->total_out = s->z.total_out;
>  	s->total_in = s->z.total_in;
>  	s->next_in = s->z.next_in;

> > So I dunno. This approach is a _lot_ more convenient than trying to
> > rebuild all the dependencies from scratch, and it runs way faster than
> > valgrind.  It did find the cases that led to the patches in this
> > series, and at least one more: if the lstat() at the end of
> > entry.c:write_entry() fails, we write nonsense into the cache_entry.
> 
> Yeah valgrind found that one too, as I tried (and apparently failed :))
> to explain in the cover letter.  I just haven't found the time yet to
> actually try and go fix that one.

> On 05 Oct 2017, at 05:46, Jeff King <peff@peff.net> wrote:
> 
> On Wed, Oct 04, 2017 at 08:30:05PM +0100, Thomas Gummerer wrote:
> 
>>> So I dunno. This approach is a _lot_ more convenient than trying to
>>> rebuild all the dependencies from scratch, and it runs way faster than
>>> valgrind.  It did find the cases that led to the patches in this
>>> series, and at least one more: if the lstat() at the end of
>>> entry.c:write_entry() fails, we write nonsense into the cache_entry.
>> 
>> Yeah valgrind found that one too, as I tried (and apparently failed :))
>> to explain in the cover letter.  I just haven't found the time yet to
>> actually try and go fix that one.
> 
> No, I just have poor memory. :)
> 
> The obvious fix is that we should check the return value of `lstat`, but
> the bigger question is why and when that would fail.
> 
> The case triggered by t0021 is using the new "delayed" filter mechanism.
> So at the time that write_entry() finishes, we don't actually have the
> file in the filesystem. I think we need to recognize that we got delayed
> and didn't actually check anything out, and skip that whole "if
> (state->refresh_cache)" block. It's not clear to me, though, how we tell
> the difference between the delayed and normal cases in that function.