git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Thomas Gummerer <t.gummerer@gmail.com>
To: git@vger.kernel.org
Cc: "Thomas Gummerer" <t.gummerer@gmail.com>,
	"Lars Schneider" <larsxschneider@gmail.com>,
	"Ben Peart" <peartben@gmail.com>,
	"Ben Peart" <benpeart@microsoft.com>,
	"Junio C Hamano" <gitster@pobox.com>,
	"Jakub Narębski" <jnareb@gmail.com>
Subject: [PATCH 3/3] sub-process: allocate argv on the heap
Date: Tue,  3 Oct 2017 20:57:13 +0100	[thread overview]
Message-ID: <20171003195713.13395-4-t.gummerer@gmail.com> (raw)
In-Reply-To: <20171003195713.13395-1-t.gummerer@gmail.com>

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.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 sub-process.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

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;
-- 
2.14.1.480.gb18f417b89


  parent reply	other threads:[~2017-10-03 19:56 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-10-03 19:57 [PATCH 0/3] fixes for running the test suite with --valgrind Thomas Gummerer
2017-10-03 19:57 ` [PATCH 1/3] path.c: fix uninitialized memory access Thomas Gummerer
2017-10-03 22:45   ` Jonathan Nieder
2017-10-03 23:30     ` Jeff King
2017-10-03 23:37       ` Jonathan Nieder
2017-10-04  4:47         ` Junio C Hamano
2017-10-04  5:21           ` Jeff King
2017-10-04 19:22           ` Thomas Gummerer
2017-10-04 19:36           ` Jonathan Nieder
2017-10-03 19:57 ` [PATCH 2/3] http-push: fix construction of hex value from path Thomas Gummerer
2017-10-03 22:53   ` Jonathan Nieder
2017-10-03 23:36     ` Jeff King
2017-10-04  4:48       ` Junio C Hamano
2017-10-04  5:20         ` Junio C Hamano
2017-10-04  5:26           ` Jeff King
2017-10-04  6:26             ` Junio C Hamano
2017-10-03 19:57 ` Thomas Gummerer [this message]
2017-10-03 20:24   ` [PATCH 3/3] sub-process: allocate argv on the heap Johannes Sixt
2017-10-04  4:59     ` Junio C Hamano
2017-10-04  5:32       ` Jeff King
2017-10-04  5:58       ` Johannes Sixt
2017-10-04 19:31       ` Thomas Gummerer
2017-10-03 20:25   ` Stefan Beller
2017-10-03 23:41 ` [PATCH 0/3] fixes for running the test suite with --valgrind Jeff King
2017-10-03 23:50   ` Jonathan Nieder
2017-10-03 23:54     ` Jeff King
2017-10-04 10:19   ` playing with MSan, was " Jeff King
2017-10-04 19:30     ` Thomas Gummerer
2017-10-05  3:46       ` lstat-ing delayed-filter output, was Re: playing with MSan Jeff King
2017-10-05 10:47         ` Lars Schneider

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20171003195713.13395-4-t.gummerer@gmail.com \
    --to=t.gummerer@gmail.com \
    --cc=benpeart@microsoft.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jnareb@gmail.com \
    --cc=larsxschneider@gmail.com \
    --cc=peartben@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).