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>,
	"Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>,
	"Junio C Hamano" <gitster@pobox.com>
Subject: [PATCH 1/3] path.c: fix uninitialized memory access
Date: Tue,  3 Oct 2017 20:57:11 +0100	[thread overview]
Message-ID: <20171003195713.13395-2-t.gummerer@gmail.com> (raw)
In-Reply-To: <20171003195713.13395-1-t.gummerer@gmail.com>

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 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(-)

diff --git a/path.c b/path.c
index b533ec938d..12f41ee877 100644
--- a/path.c
+++ b/path.c
@@ -34,20 +34,21 @@ static struct strbuf *get_pathname(void)
 	return sb;
 }
 
-static char *cleanup_path(char *path)
+static char *cleanup_path(char *path, int len)
 {
 	/* Clean it up */
-	if (!memcmp(path, "./", 2)) {
-		path += 2;
-		while (*path == '/')
-			path++;
+	int skip = 0;
+	if (len >= 2 && !memcmp(path, "./", 2)) {
+		skip += 2;
+		while (skip < len && *(path + skip) == '/')
+			skip++;
 	}
-	return path;
+	return path + skip;
 }
 
 static void strbuf_cleanup_path(struct strbuf *sb)
 {
-	char *path = cleanup_path(sb->buf);
+	char *path = cleanup_path(sb->buf, sb->len);
 	if (path > sb->buf)
 		strbuf_remove(sb, 0, path - sb->buf);
 }
@@ -64,7 +65,7 @@ char *mksnpath(char *buf, size_t n, const char *fmt, ...)
 		strlcpy(buf, bad_path, n);
 		return buf;
 	}
-	return cleanup_path(buf);
+	return cleanup_path(buf, n);
 }
 
 static int dir_prefix(const char *buf, const char *dir)
@@ -494,7 +495,7 @@ const char *mkpath(const char *fmt, ...)
 	va_start(args, fmt);
 	strbuf_vaddf(pathname, fmt, args);
 	va_end(args);
-	return cleanup_path(pathname->buf);
+	return cleanup_path(pathname->buf, pathname->len);
 }
 
 const char *worktree_git_path(const struct worktree *wt, const char *fmt, ...)
-- 
2.14.1.480.gb18f417b89


  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 ` Thomas Gummerer [this message]
2017-10-03 22:45   ` [PATCH 1/3] path.c: fix uninitialized memory access 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 ` [PATCH 3/3] sub-process: allocate argv on the heap Thomas Gummerer
2017-10-03 20:24   ` 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-2-t.gummerer@gmail.com \
    --to=t.gummerer@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=pclouds@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).