{"thread":{"id":"46893","subject":"[PATCH 0/3] fixes for running the test suite with --valgrind","startedAt":"2017-10-03T19:56:32Z","lastAt":"2017-10-05T10:47:22Z","messageCount":30,"participants":["Thomas Gummerer","Johannes Sixt","Stefan Beller","Jonathan Nieder","Jeff King","Junio C Hamano","Lars Schneider"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"329588","messageId":"20171003195713.13395-1-t.gummerer@gmail.com","threadId":"46893","inReplyTo":null,"subject":"[PATCH 0/3] fixes for running the test suite with --valgrind","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2017-10-03T19:57:10Z","receivedAt":"2017-10-03T19:56:32Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"I recently tried to run the git test suite with --valgrind.\nUnfortunately it didn't come back completely clean.  This patch series\nfixes a bunch of these errors, although unfortunately not quite all of\nthem yet.\n\nThe remaining failures are in\n\nt0021.15 - This one is not actually valgrind complaining about\nsomething, but the clean/smudge script not writing debug.log for some\nreason.  I'm not quite sure what exactly is going on here.\nt0021.25, t0021.26 - This is an actual uninitialized memory usage:\n\n==4751== Conditional jump or move depends on uninitialised value(s)\n==4751==    at 0x2796D5: fill_stat_cache_info (read-cache.c:153)\n==4751==    by 0x2218A2: write_entry (entry.c:359)\n==4751==    by 0x221D42: checkout_entry (entry.c:458)\n==4751==    by 0x2EB627: check_updates (unpack-trees.c:382)\n==4751==    by 0x2EDBA1: unpack_trees (unpack-trees.c:1380)\n==4751==    by 0x13797E: checkout (clone.c:750)\n==4751==    by 0x138FF4: cmd_clone (clone.c:1194)\n==4751==    by 0x11A6F2: run_builtin (git.c:342)\n==4751==    by 0x11A9E8: handle_builtin (git.c:550)\n==4751==    by 0x11ABCC: run_argv (git.c:602)\n==4751==    by 0x11AD8E: cmd_main (git.c:679)\n==4751==    by 0x1BF125: main (common-main.c:43)\n==4751==  Uninitialised value was created by a stack allocation\n==4751==    at 0x2212B4: write_entry (entry.c:254)\n==4751== \n\nSo far I've tracked this one down to the lstat call in write_entry\nfailing, and thus not filling struct stat_info.  I'm not quite sure\nyet about the best workaround for that (and I'm not very familiar with\nthe clean/smudge code).  I'll keep digging what the problem there is.\n\nThere's also one test that's unexpectedly passing when the test suite\nis run under valgrind (t6410.) but I haven't dug into what's up with\nthat yet.\n\nThese patches could be applied by themselves as well, but since they\nwork toward the same goal, and a cover letter would explain where\nthese are coming from I decided to make them into a patch series.\n\nThomas Gummerer (3):\n  path.c: fix uninitialized memory access\n  http-push: fix construction of hex value from path\n  sub-process: allocate argv on the heap\n\n http-push.c   |  2 +-\n path.c        | 19 ++++++++++---------\n sub-process.c |  4 +++-\n 3 files changed, 14 insertions(+), 11 deletions(-)\n\n-- \n2.14.1.480.gb18f417b89\n\n"},{"id":"329589","messageId":"20171003195713.13395-2-t.gummerer@gmail.com","threadId":"46893","inReplyTo":"20171003195713.13395-1-t.gummerer@gmail.com","subject":"[PATCH 1/3] path.c: fix uninitialized memory access","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2017-10-03T19:57:11Z","receivedAt":"2017-10-03T19:56:36Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"In cleanup_path we're passing in a char array, run a memcmp on it, and\nrun through it without ever checking if something is in the array in the\nfirst place.  This can lead us to access uninitialized memory, for\nexample in t5541-http-push-smart.sh test 7, when run under valgrind:\n\n==4423== Conditional jump or move depends on uninitialised value(s)\n==4423==    at 0x242FA9: cleanup_path (path.c:35)\n==4423==    by 0x242FA9: mkpath (path.c:456)\n==4423==    by 0x256CC7: refname_match (refs.c:364)\n==4423==    by 0x26C181: count_refspec_match (remote.c:1015)\n==4423==    by 0x26C181: match_explicit_lhs (remote.c:1126)\n==4423==    by 0x26C181: check_push_refs (remote.c:1409)\n==4423==    by 0x2ABB4D: transport_push (transport.c:870)\n==4423==    by 0x186703: push_with_options (push.c:332)\n==4423==    by 0x18746D: do_push (push.c:409)\n==4423==    by 0x18746D: cmd_push (push.c:566)\n==4423==    by 0x1183E0: run_builtin (git.c:352)\n==4423==    by 0x11973E: handle_builtin (git.c:539)\n==4423==    by 0x11973E: run_argv (git.c:593)\n==4423==    by 0x11973E: main (git.c:698)\n==4423==  Uninitialised value was created by a heap allocation\n==4423==    at 0x4C2CD8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)\n==4423==    by 0x4C2F195: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)\n==4423==    by 0x2C196B: xrealloc (wrapper.c:137)\n==4423==    by 0x29A30B: strbuf_grow (strbuf.c:66)\n==4423==    by 0x29A30B: strbuf_vaddf (strbuf.c:277)\n==4423==    by 0x242F9F: mkpath (path.c:454)\n==4423==    by 0x256CC7: refname_match (refs.c:364)\n==4423==    by 0x26C181: count_refspec_match (remote.c:1015)\n==4423==    by 0x26C181: match_explicit_lhs (remote.c:1126)\n==4423==    by 0x26C181: check_push_refs (remote.c:1409)\n==4423==    by 0x2ABB4D: transport_push (transport.c:870)\n==4423==    by 0x186703: push_with_options (push.c:332)\n==4423==    by 0x18746D: do_push (push.c:409)\n==4423==    by 0x18746D: cmd_push (push.c:566)\n==4423==    by 0x1183E0: run_builtin (git.c:352)\n==4423==    by 0x11973E: handle_builtin (git.c:539)\n==4423==    by 0x11973E: run_argv (git.c:593)\n==4423==    by 0x11973E: main (git.c:698)\n==4423==\n\nAvoid this by checking passing in the length of the string in the char\narray, and checking that we never run over it.\n\nSigned-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n---\n path.c | 19 ++++++++++---------\n 1 file changed, 10 insertions(+), 9 deletions(-)\n\ndiff --git a/path.c b/path.c\nindex b533ec938d..12f41ee877 100644\n--- a/path.c\n+++ b/path.c\n@@ -34,20 +34,21 @@ static struct strbuf *get_pathname(void)\n \treturn sb;\n }\n \n-static char *cleanup_path(char *path)\n+static char *cleanup_path(char *path, int len)\n {\n \t/* Clean it up */\n-\tif (!memcmp(path, \"./\", 2)) {\n-\t\tpath += 2;\n-\t\twhile (*path == '/')\n-\t\t\tpath++;\n+\tint skip = 0;\n+\tif (len >= 2 && !memcmp(path, \"./\", 2)) {\n+\t\tskip += 2;\n+\t\twhile (skip < len && *(path + skip) == '/')\n+\t\t\tskip++;\n \t}\n-\treturn path;\n+\treturn path + skip;\n }\n \n static void strbuf_cleanup_path(struct strbuf *sb)\n {\n-\tchar *path = cleanup_path(sb->buf);\n+\tchar *path = cleanup_path(sb->buf, sb->len);\n \tif (path > sb->buf)\n \t\tstrbuf_remove(sb, 0, path - sb->buf);\n }\n@@ -64,7 +65,7 @@ char *mksnpath(char *buf, size_t n, const char *fmt, ...)\n \t\tstrlcpy(buf, bad_path, n);\n \t\treturn buf;\n \t}\n-\treturn cleanup_path(buf);\n+\treturn cleanup_path(buf, n);\n }\n \n static int dir_prefix(const char *buf, const char *dir)\n@@ -494,7 +495,7 @@ const char *mkpath(const char *fmt, ...)\n \tva_start(args, fmt);\n \tstrbuf_vaddf(pathname, fmt, args);\n \tva_end(args);\n-\treturn cleanup_path(pathname->buf);\n+\treturn cleanup_path(pathname->buf, pathname->len);\n }\n \n const char *worktree_git_path(const struct worktree *wt, const char *fmt, ...)\n-- \n2.14.1.480.gb18f417b89\n\n"},{"id":"329590","messageId":"20171003195713.13395-3-t.gummerer@gmail.com","threadId":"46893","inReplyTo":"20171003195713.13395-1-t.gummerer@gmail.com","subject":"[PATCH 2/3] http-push: fix construction of hex value from path","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2017-10-03T19:57:12Z","receivedAt":"2017-10-03T19:56:40Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"The get_oid_hex_from_objpath takes care of creating a oid from a\npathname.  It does this by memcpy'ing the first two bytes of the path to\nthe \"hex\" string, then skipping the '/', and then copying the rest of the\npath to the \"hex\" string.  Currently it fails to increase the pointer to\nthe hex string, so the second memcpy invocation just mashes over what\nwas copied in the first one, and leaves the last two bytes in the string\nuninitialized.\n\nThis breaks valgrind in t5540, although the test passes without\nvalgrind:\n\n==5490== Use of uninitialised value of size 8\n==5490==    at 0x13C6B5: hexval (cache.h:1238)\n==5490==    by 0x13C6DB: hex2chr (cache.h:1247)\n==5490==    by 0x13C734: get_sha1_hex (hex.c:42)\n==5490==    by 0x13C78E: get_oid_hex (hex.c:53)\n==5490==    by 0x118BDA: get_oid_hex_from_objpath (http-push.c:1023)\n==5490==    by 0x118C92: process_ls_object (http-push.c:1038)\n==5490==    by 0x118E5B: handle_remote_ls_ctx (http-push.c:1077)\n==5490==    by 0x118227: xml_end_tag (http-push.c:815)\n==5490==    by 0x50C1448: ??? (in /usr/lib/libexpat.so.1.6.6)\n==5490==    by 0x50C221B: ??? (in /usr/lib/libexpat.so.1.6.6)\n==5490==    by 0x50BFBF2: ??? (in /usr/lib/libexpat.so.1.6.6)\n==5490==    by 0x50C0B24: ??? (in /usr/lib/libexpat.so.1.6.6)\n==5490==  Uninitialised value was created by a stack allocation\n==5490==    at 0x118B63: get_oid_hex_from_objpath (http-push.c:1012)\n==5490==\n\nFix this by correctly incrementing the pointer to the \"hex\" variable, so\nthe first two bytes are left untouched by the memcpy call, and the last\ntwo bytes are correctly initialized.\n\nSigned-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n---\n http-push.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/http-push.c b/http-push.c\nindex e4c9b065ce..e9a01ec4da 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -1018,7 +1018,7 @@ static int get_oid_hex_from_objpath(const char *path, struct object_id *oid)\n \tmemcpy(hex, path, 2);\n \tpath += 2;\n \tpath++; /* skip '/' */\n-\tmemcpy(hex, path, GIT_SHA1_HEXSZ - 2);\n+\tmemcpy(hex + 2, path, GIT_SHA1_HEXSZ - 2);\n \n \treturn get_oid_hex(hex, oid);\n }\n-- \n2.14.1.480.gb18f417b89\n\n"},{"id":"329591","messageId":"20171003195713.13395-4-t.gummerer@gmail.com","threadId":"46893","inReplyTo":"20171003195713.13395-1-t.gummerer@gmail.com","subject":"[PATCH 3/3] sub-process: allocate argv on the heap","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2017-10-03T19:57:13Z","receivedAt":"2017-10-03T19:56:42Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Currently the argv is only allocated on the stack, and then assigned to\nprocess->argv.  When the start_subprocess function goes out of scope,\nthe local argv variable is eliminated from the stack, but the pointer is\nstill kept around in process->argv.\n\nMuch later when we try to access the same process->argv in\nfinish_command, this leads us to access a memory location that no longer\ncontains what we want.  As argv0 is only used for printing errors, this\nis not easily noticed in normal git operations.  However when running\nt0021-conversion.sh through valgrind, valgrind rightfully complains:\n\n==21024== Invalid read of size 8\n==21024==    at 0x2ACF64: finish_command (run-command.c:869)\n==21024==    by 0x2D6B18: subprocess_exit_handler (sub-process.c:72)\n==21024==    by 0x2AB41E: cleanup_children (run-command.c:45)\n==21024==    by 0x2AB526: cleanup_children_on_exit (run-command.c:81)\n==21024==    by 0x54AD487: __run_exit_handlers (in /usr/lib/libc-2.26.so)\n==21024==    by 0x54AD4D9: exit (in /usr/lib/libc-2.26.so)\n==21024==    by 0x11A9EF: handle_builtin (git.c:550)\n==21024==    by 0x11ABCC: run_argv (git.c:602)\n==21024==    by 0x11AD8E: cmd_main (git.c:679)\n==21024==    by 0x1BF125: main (common-main.c:43)\n==21024==  Address 0x1ffeffec00 is on thread 1's stack\n==21024==  1504 bytes below stack pointer\n==21024==\n\nFix this by allocating the memory on properly on the heap.  This memory\nis allocated on the heap, and never free'd.  However the same seems to be\ntrue for struct child_process, so it should be fine to just let the\nmemory be free'd when the process terminates.\n\nSigned-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n---\n sub-process.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/sub-process.c b/sub-process.c\nindex 6dde5062be..4680af8193 100644\n--- a/sub-process.c\n+++ b/sub-process.c\n@@ -77,7 +77,9 @@ int subprocess_start(struct hashmap *hashmap, struct subprocess_entry *entry, co\n {\n \tint err;\n \tstruct child_process *process;\n-\tconst char *argv[] = { cmd, NULL };\n+\tconst char **argv = xmalloc(2 * sizeof(char *));\n+\targv[0] = cmd;\n+\targv[1] = NULL;\n \n \tentry->cmd = cmd;\n \tprocess = &entry->process;\n-- \n2.14.1.480.gb18f417b89\n\n"},{"id":"329603","messageId":"85456f81-9e9f-d0e4-86f7-9f546691dee3@kdbg.org","threadId":"46893","inReplyTo":"20171003195713.13395-4-t.gummerer@gmail.com","subject":"Re: [PATCH 3/3] sub-process: allocate argv on the heap","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2017-10-03T20:24:57Z","receivedAt":"2017-10-03T20:25:08Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 03.10.2017 um 21:57 schrieb Thomas Gummerer:\n> diff --git a/sub-process.c b/sub-process.c\n> index 6dde5062be..4680af8193 100644\n> --- a/sub-process.c\n> +++ b/sub-process.c\n> @@ -77,7 +77,9 @@ int subprocess_start(struct hashmap *hashmap, struct subprocess_entry *entry, co\n>   {\n>   \tint err;\n>   \tstruct child_process *process;\n> -\tconst char *argv[] = { cmd, NULL };\n> +\tconst char **argv = xmalloc(2 * sizeof(char *));\n> +\targv[0] = cmd;\n> +\targv[1] = NULL;\n>   \n>   \tentry->cmd = cmd;\n>   \tprocess = &entry->process;\n> \n\nPerhaps this should become\n\n\targv_array_push(&process->args, cmd);\n\nso that there is no new memory leak?\n\n-- Hannes\n"},{"id":"329604","messageId":"CAGZ79kYPEeEh1ZG6M7bjMkA6V_XsU2LZCqdD0OxXU0h4p17FOQ@mail.gmail.com","threadId":"46893","inReplyTo":"20171003195713.13395-4-t.gummerer@gmail.com","subject":"Re: [PATCH 3/3] sub-process: allocate argv on the heap","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-03T20:25:33Z","receivedAt":"2017-10-03T20:25:39Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Oct 3, 2017 at 12:57 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:\n> Currently the argv is only allocated on the stack, and then assigned to\n> process->argv.  When the start_subprocess function goes out of scope,\n> the local argv variable is eliminated from the stack, but the pointer is\n> still kept around in process->argv.\n>\n> Much later when we try to access the same process->argv in\n> finish_command, this leads us to access a memory location that no longer\n> contains what we want.  As argv0 is only used for printing errors, this\n> is not easily noticed in normal git operations.  However when running\n> t0021-conversion.sh through valgrind, valgrind rightfully complains:\n>\n> ==21024== Invalid read of size 8\n> ==21024==    at 0x2ACF64: finish_command (run-command.c:869)\n> ==21024==    by 0x2D6B18: subprocess_exit_handler (sub-process.c:72)\n> ==21024==    by 0x2AB41E: cleanup_children (run-command.c:45)\n> ==21024==    by 0x2AB526: cleanup_children_on_exit (run-command.c:81)\n> ==21024==    by 0x54AD487: __run_exit_handlers (in /usr/lib/libc-2.26.so)\n> ==21024==    by 0x54AD4D9: exit (in /usr/lib/libc-2.26.so)\n> ==21024==    by 0x11A9EF: handle_builtin (git.c:550)\n> ==21024==    by 0x11ABCC: run_argv (git.c:602)\n> ==21024==    by 0x11AD8E: cmd_main (git.c:679)\n> ==21024==    by 0x1BF125: main (common-main.c:43)\n> ==21024==  Address 0x1ffeffec00 is on thread 1's stack\n> ==21024==  1504 bytes below stack pointer\n> ==21024==\n>\n> Fix this by allocating the memory on properly on the heap.  This memory\n> is allocated on the heap, and never free'd.  However the same seems to be\n> true for struct child_process, so it should be fine to just let the\n> memory be free'd when the process terminates.\n\nUh. :( The broken window theory at work.\n\nThe patch below seems correct, but as you eluded to, now we'd be\nleaking memory. The run_command API has two fields 'char **argv'\nand 'argv_array args'. The argv is kept around for historical reasons\nas well as when the caller wants to be in control of the array (the caller\nneeds to free the memory, but could also just reuse it for a slightly\ndifferent invocation), whereas the args argument is owned by the child\nprocess, such that the memory is freed by finish_command.\n\nAs we're doing a memory allocation now anyway, how about:\n\n-       const char *argv[] = { cmd, NULL };\n...\n    child_process_init(process);\n+    argv_array_push(process.args, cmd);\n"},{"id":"329614","messageId":"20171003224501.GD19555@aiede.mtv.corp.google.com","threadId":"46893","inReplyTo":"20171003195713.13395-2-t.gummerer@gmail.com","subject":"Re: [PATCH 1/3] path.c: fix uninitialized memory access","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-10-03T22:45:01Z","receivedAt":"2017-10-03T22:45:10Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nThomas Gummerer wrote:\n\n> In cleanup_path we're passing in a char array, run a memcmp on it, and\n> run through it without ever checking if something is in the array in the\n> first place.  This can lead us to access uninitialized memory, for\n> example in t5541-http-push-smart.sh test 7, when run under valgrind:\n[...]\n> Avoid this by checking passing in the length of the string in the char\n> array, and checking that we never run over it.\n>\n> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n> ---\n>  path.c | 19 ++++++++++---------\n>  1 file changed, 10 insertions(+), 9 deletions(-)\n\nWhen I first read the above, I thought it was going to be about a\nNUL-terminated string that was missing a NUL.  But in fact, the issue\nis that strlen(path) can be < 2.\n\nIn other words, an alternative fix would be\n\n\tif (*path == '.' && path[1] == '/') {\n\t\t...\n\t}\n\nwhich would not require passing in 'len' or switching to index-based\narithmetic.  I think I prefer it.  What do you think?\n\nThanks and hope that helps,\nJonathan\n\ndiff --git i/path.c w/path.c\nindex b533ec938d..3a1fbee1e0 100644\n--- i/path.c\n+++ w/path.c\n@@ -37,7 +37,7 @@ static struct strbuf *get_pathname(void)\n static char *cleanup_path(char *path)\n {\n \t/* Clean it up */\n-\tif (!memcmp(path, \"./\", 2)) {\n+\tif (*path == '.' && path[1] == '/') {\n \t\tpath += 2;\n \t\twhile (*path == '/')\n \t\t\tpath++;\n"},{"id":"329615","messageId":"20171003225315.GE19555@aiede.mtv.corp.google.com","threadId":"46893","inReplyTo":"20171003195713.13395-3-t.gummerer@gmail.com","subject":"Re: [PATCH 2/3] http-push: fix construction of hex value from path","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-10-03T22:53:15Z","receivedAt":"2017-10-03T22:53:24Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nThomas Gummerer wrote:\n\n> The get_oid_hex_from_objpath takes care of creating a oid from a\n> pathname.  It does this by memcpy'ing the first two bytes of the path to\n> the \"hex\" string, then skipping the '/', and then copying the rest of the\n> path to the \"hex\" string.  Currently it fails to increase the pointer to\n> the hex string, so the second memcpy invocation just mashes over what\n> was copied in the first one, and leaves the last two bytes in the string\n> uninitialized.\n\nWow.  The fix is obviously correct.\n\n> This breaks valgrind in t5540, although the test passes without\n> valgrind:\n[...]\n> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n> ---\n>  http-push.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n\nWould it be straightforward to add a correctness test for this?  It\nseems like this code path didn't work at all and no one noticed.\n\nThis is the code path in http-push.c which says\n\n /*\n  * NEEDSWORK: remote_ls() ignores info/refs on the remote side.  But it\n  * should _only_ heed the information from that file, instead of trying to\n  * determine the refs from the remote file system (badly: it does not even\n  * know about packed-refs).\n  */\n static void remote_ls(const char *path, int flags,\n\nI think the problem is that when it fails, we end up thinking that\nthere are *fewer* objects than are actually present remotely so the\nonly ill effect is pushing too much.  So this should be observable in\nserver logs (i.e. it is testable) but it's not a catastrophic failure\nwhich means it's harder to test than it would be otherwise.\n\nMoreover, this is in the webdav-based \"dumb http\" push code path,\nwhich I do not trust much at all.  I wonder if we could retire it\ncompletely (or at least provide an option to turn it off).\n\n> diff --git a/http-push.c b/http-push.c\n> index e4c9b065ce..e9a01ec4da 100644\n> --- a/http-push.c\n> +++ b/http-push.c\n> @@ -1018,7 +1018,7 @@ static int get_oid_hex_from_objpath(const char *path, struct object_id *oid)\n>  \tmemcpy(hex, path, 2);\n>  \tpath += 2;\n>  \tpath++; /* skip '/' */\n> -\tmemcpy(hex, path, GIT_SHA1_HEXSZ - 2);\n> +\tmemcpy(hex + 2, path, GIT_SHA1_HEXSZ - 2);\n>  \n>  \treturn get_oid_hex(hex, oid);\n\nThanks,\nJonathan\n"},{"id":"329618","messageId":"20171003233040.2ws6kny3e4kypfub@sigill.intra.peff.net","threadId":"46893","inReplyTo":"20171003224501.GD19555@aiede.mtv.corp.google.com","subject":"Re: [PATCH 1/3] path.c: fix uninitialized memory access","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-03T23:30:40Z","receivedAt":"2017-10-03T23:30:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 03, 2017 at 03:45:01PM -0700, Jonathan Nieder wrote:\n\n> When I first read the above, I thought it was going to be about a\n> NUL-terminated string that was missing a NUL.  But in fact, the issue\n> is that strlen(path) can be < 2.\n> \n> In other words, an alternative fix would be\n> \n> \tif (*path == '.' && path[1] == '/') {\n> \t\t...\n> \t}\n> \n> which would not require passing in 'len' or switching to index-based\n> arithmetic.  I think I prefer it.  What do you think?\n\nYes, I think that approach is much nicer. I think you could even use\nskip_prefix. Unfortunately you have to play a few games with const-ness,\nbut I think the resulting signature for cleanup_path() is an\nimprovement:\n\ndiff --git a/path.c b/path.c\nindex 00ec04e7a5..2e09a7bce0 100644\n--- a/path.c\n+++ b/path.c\n@@ -34,11 +34,10 @@ static struct strbuf *get_pathname(void)\n \treturn sb;\n }\n \n-static char *cleanup_path(char *path)\n+static const char *cleanup_path(const char *path)\n {\n \t/* Clean it up */\n-\tif (!memcmp(path, \"./\", 2)) {\n-\t\tpath += 2;\n+\tif (skip_prefix(path, \"./\", &path)) {\n \t\twhile (*path == '/')\n \t\t\tpath++;\n \t}\n@@ -47,7 +46,7 @@ static char *cleanup_path(char *path)\n \n static void strbuf_cleanup_path(struct strbuf *sb)\n {\n-\tchar *path = cleanup_path(sb->buf);\n+\tconst char *path = cleanup_path(sb->buf);\n \tif (path > sb->buf)\n \t\tstrbuf_remove(sb, 0, path - sb->buf);\n }\n@@ -64,7 +63,7 @@ char *mksnpath(char *buf, size_t n, const char *fmt, ...)\n \t\tstrlcpy(buf, bad_path, n);\n \t\treturn buf;\n \t}\n-\treturn cleanup_path(buf);\n+\treturn (char *)cleanup_path(buf);\n }\n \n static int dir_prefix(const char *buf, const char *dir)\n"},{"id":"329619","messageId":"20171003233638.fq6lgls2qsucfbn3@sigill.intra.peff.net","threadId":"46893","inReplyTo":"20171003225315.GE19555@aiede.mtv.corp.google.com","subject":"Re: [PATCH 2/3] http-push: fix construction of hex value from path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-03T23:36:38Z","receivedAt":"2017-10-03T23:36:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 03, 2017 at 03:53:15PM -0700, Jonathan Nieder wrote:\n\n> Thomas Gummerer wrote:\n> \n> > The get_oid_hex_from_objpath takes care of creating a oid from a\n> > pathname.  It does this by memcpy'ing the first two bytes of the path to\n> > the \"hex\" string, then skipping the '/', and then copying the rest of the\n> > path to the \"hex\" string.  Currently it fails to increase the pointer to\n> > the hex string, so the second memcpy invocation just mashes over what\n> > was copied in the first one, and leaves the last two bytes in the string\n> > uninitialized.\n> \n> Wow.  The fix is obviously correct.\n\nAgreed. This is brown-paper-bag worthy. Thanks, Thomas, for cleaning up\nmy mess.\n\n> I think the problem is that when it fails, we end up thinking that\n> there are *fewer* objects than are actually present remotely so the\n> only ill effect is pushing too much.  So this should be observable in\n> server logs (i.e. it is testable) but it's not a catastrophic failure\n> which means it's harder to test than it would be otherwise.\n\nAnd thank you, Jonathan, for this analysis. I had also wondered how such\na frequently-triggered bug could have gone completely unnoticed, but\nthis explains it.\n\n> Moreover, this is in the webdav-based \"dumb http\" push code path,\n> which I do not trust much at all.  I wonder if we could retire it\n> completely (or at least provide an option to turn it off).\n\nI would really like that, too. It has been the cause of a lot of pain\nwhen working with the smart code, and I am not at all surprised to find\na bug of this magnitude lurking in it. I'd _hoped_ this could show that\nthe system has been unusably broken for years, which would give us\nconfidence to turn it off. :) But per your paragraph above, people could\nvery easily still have been happily using it in the meantime.\n\n-Peff\n"},{"id":"329620","messageId":"20171003233706.GH19555@aiede.mtv.corp.google.com","threadId":"46893","inReplyTo":"20171003233040.2ws6kny3e4kypfub@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] path.c: fix uninitialized memory access","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-10-03T23:37:06Z","receivedAt":"2017-10-03T23:37:14Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Tue, Oct 03, 2017 at 03:45:01PM -0700, Jonathan Nieder wrote:\n\n>> In other words, an alternative fix would be\n>> \n>> \tif (*path == '.' && path[1] == '/') {\n>> \t\t...\n>> \t}\n>> \n>> which would not require passing in 'len' or switching to index-based\n>> arithmetic.  I think I prefer it.  What do you think?\n>\n> Yes, I think that approach is much nicer. I think you could even use\n> skip_prefix. Unfortunately you have to play a few games with const-ness,\n> but I think the resulting signature for cleanup_path() is an\n> improvement:\n\nOoh!\n\nFor what it's worth, if you add a commit message with Thomas's\nReported-by then this lgtm.\n\nThanks,\nJonathan\n\n> diff --git a/path.c b/path.c\n> index 00ec04e7a5..2e09a7bce0 100644\n> --- a/path.c\n> +++ b/path.c\n> @@ -34,11 +34,10 @@ static struct strbuf *get_pathname(void)\n>  \treturn sb;\n>  }\n>  \n> -static char *cleanup_path(char *path)\n> +static const char *cleanup_path(const char *path)\n>  {\n>  \t/* Clean it up */\n> -\tif (!memcmp(path, \"./\", 2)) {\n> -\t\tpath += 2;\n> +\tif (skip_prefix(path, \"./\", &path)) {\n>  \t\twhile (*path == '/')\n>  \t\t\tpath++;\n>  \t}\n> @@ -47,7 +46,7 @@ static char *cleanup_path(char *path)\n>  \n>  static void strbuf_cleanup_path(struct strbuf *sb)\n>  {\n> -\tchar *path = cleanup_path(sb->buf);\n> +\tconst char *path = cleanup_path(sb->buf);\n>  \tif (path > sb->buf)\n>  \t\tstrbuf_remove(sb, 0, path - sb->buf);\n>  }\n> @@ -64,7 +63,7 @@ char *mksnpath(char *buf, size_t n, const char *fmt, ...)\n>  \t\tstrlcpy(buf, bad_path, n);\n>  \t\treturn buf;\n>  \t}\n> -\treturn cleanup_path(buf);\n> +\treturn (char *)cleanup_path(buf);\n>  }\n>  \n>  static int dir_prefix(const char *buf, const char *dir)\n"},{"id":"329621","messageId":"20171003234153.pmslizidgbifl6en@sigill.intra.peff.net","threadId":"46893","inReplyTo":"20171003195713.13395-1-t.gummerer@gmail.com","subject":"Re: [PATCH 0/3] fixes for running the test suite with --valgrind","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-03T23:41:54Z","receivedAt":"2017-10-03T23:42:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 03, 2017 at 08:57:10PM +0100, Thomas Gummerer wrote:\n\n> I recently tried to run the git test suite with --valgrind.\n> Unfortunately it didn't come back completely clean.  This patch series\n> fixes a bunch of these errors, although unfortunately not quite all of\n> them yet.\n\nThanks for trying that. I've largely switched to ASan over valgrind\nbecause it runs _so_ much faster. Unfortunately it seems to miss\nthese uninitialized-memory cases.\n\nI think using SANITIZE=memory would catch these, but it needs some\nsuppressions tuning. The weird \"zlib reads uninitialized memory\" error\nis a problem (valgrind sees this, too, but we have suppressions).\n\n-Peff\n"},{"id":"329624","messageId":"20171003235058.GI19555@aiede.mtv.corp.google.com","threadId":"46893","inReplyTo":"20171003234153.pmslizidgbifl6en@sigill.intra.peff.net","subject":"Re: [PATCH 0/3] fixes for running the test suite with --valgrind","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-10-03T23:50:58Z","receivedAt":"2017-10-03T23:51:05Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> I think using SANITIZE=memory would catch these, but it needs some\n> suppressions tuning. The weird \"zlib reads uninitialized memory\" error\n> is a problem (valgrind sees this, too, but we have suppressions).\n\nWhat version of zlib do you use?  I've heard some good things about\nv1.2.11 improving matters, though I haven't checked yet.\n\nThanks,\nJonathan\n"},{"id":"329626","messageId":"20171003235426.fnlyt45z56iquqin@sigill.intra.peff.net","threadId":"46893","inReplyTo":"20171003235058.GI19555@aiede.mtv.corp.google.com","subject":"Re: [PATCH 0/3] fixes for running the test suite with --valgrind","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-03T23:54:26Z","receivedAt":"2017-10-03T23:54:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 03, 2017 at 04:50:58PM -0700, Jonathan Nieder wrote:\n\n> > I think using SANITIZE=memory would catch these, but it needs some\n> > suppressions tuning. The weird \"zlib reads uninitialized memory\" error\n> > is a problem (valgrind sees this, too, but we have suppressions).\n> \n> What version of zlib do you use?  I've heard some good things about\n> v1.2.11 improving matters, though I haven't checked yet.\n\n1.2.8.dfsg-5, from Debian unstable. I'm happy to try v1.2.11 once it\nhits unstable or experimental. :)\n\n-Peff\n"},{"id":"329644","messageId":"xmqqfuazecym.fsf@gitster.mtv.corp.google.com","threadId":"46893","inReplyTo":"20171003233706.GH19555@aiede.mtv.corp.google.com","subject":"Re: [PATCH 1/3] path.c: fix uninitialized memory access","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-04T04:47:29Z","receivedAt":"2017-10-04T04:47:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> Jeff King wrote:\n>> On Tue, Oct 03, 2017 at 03:45:01PM -0700, Jonathan Nieder wrote:\n>\n>>> In other words, an alternative fix would be\n>>> \n>>> \tif (*path == '.' && path[1] == '/') {\n>>> \t\t...\n>>> \t}\n>>> \n>>> which would not require passing in 'len' or switching to index-based\n>>> arithmetic.  I think I prefer it.  What do you think?\n>>\n>> Yes, I think that approach is much nicer. I think you could even use\n>> skip_prefix. Unfortunately you have to play a few games with const-ness,\n>> but I think the resulting signature for cleanup_path() is an\n>> improvement:\n\nTo tie the loose end, here is what I'll queue.\n\n-- >8 --\nFrom: Jeff King <peff@peff.net>\nDate: Tue, 3 Oct 2017 19:30:40 -0400\nSubject: [PATCH] path.c: fix uninitialized memory access\n\nIn cleanup_path we're passing in a char array, run a memcmp on it, and\nrun through it without ever checking if something is in the array in the\nfirst place.  This can lead us to access uninitialized memory, for\nexample in t5541-http-push-smart.sh test 7, when run under valgrind:\n\n==4423== Conditional jump or move depends on uninitialised value(s)\n==4423==    at 0x242FA9: cleanup_path (path.c:35)\n==4423==    by 0x242FA9: mkpath (path.c:456)\n==4423==    by 0x256CC7: refname_match (refs.c:364)\n==4423==    by 0x26C181: count_refspec_match (remote.c:1015)\n==4423==    by 0x26C181: match_explicit_lhs (remote.c:1126)\n==4423==    by 0x26C181: check_push_refs (remote.c:1409)\n==4423==    by 0x2ABB4D: transport_push (transport.c:870)\n==4423==    by 0x186703: push_with_options (push.c:332)\n==4423==    by 0x18746D: do_push (push.c:409)\n==4423==    by 0x18746D: cmd_push (push.c:566)\n==4423==    by 0x1183E0: run_builtin (git.c:352)\n==4423==    by 0x11973E: handle_builtin (git.c:539)\n==4423==    by 0x11973E: run_argv (git.c:593)\n==4423==    by 0x11973E: main (git.c:698)\n==4423==  Uninitialised value was created by a heap allocation\n==4423==    at 0x4C2CD8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)\n==4423==    by 0x4C2F195: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)\n==4423==    by 0x2C196B: xrealloc (wrapper.c:137)\n==4423==    by 0x29A30B: strbuf_grow (strbuf.c:66)\n==4423==    by 0x29A30B: strbuf_vaddf (strbuf.c:277)\n==4423==    by 0x242F9F: mkpath (path.c:454)\n==4423==    by 0x256CC7: refname_match (refs.c:364)\n==4423==    by 0x26C181: count_refspec_match (remote.c:1015)\n==4423==    by 0x26C181: match_explicit_lhs (remote.c:1126)\n==4423==    by 0x26C181: check_push_refs (remote.c:1409)\n==4423==    by 0x2ABB4D: transport_push (transport.c:870)\n==4423==    by 0x186703: push_with_options (push.c:332)\n==4423==    by 0x18746D: do_push (push.c:409)\n==4423==    by 0x18746D: cmd_push (push.c:566)\n==4423==    by 0x1183E0: run_builtin (git.c:352)\n==4423==    by 0x11973E: handle_builtin (git.c:539)\n==4423==    by 0x11973E: run_argv (git.c:593)\n==4423==    by 0x11973E: main (git.c:698)\n==4423==\n\nAvoid this by using skip_prefix(), which knows not to go beyond the\nend of the string.\n\nReported-by: Thomas Gummerer <t.gummerer@gmail.com>\nSigned-off-by: Jeff King <peff@peff.net>\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n path.c | 9 ++++-----\n 1 file changed, 4 insertions(+), 5 deletions(-)\n\ndiff --git a/path.c b/path.c\nindex e50d2befcf..2fecf854fe 100644\n--- a/path.c\n+++ b/path.c\n@@ -33,11 +33,10 @@ static struct strbuf *get_pathname(void)\n \treturn sb;\n }\n \n-static char *cleanup_path(char *path)\n+static const char *cleanup_path(const char *path)\n {\n \t/* Clean it up */\n-\tif (!memcmp(path, \"./\", 2)) {\n-\t\tpath += 2;\n+\tif (skip_prefix(path, \"./\", &path)) {\n \t\twhile (*path == '/')\n \t\t\tpath++;\n \t}\n@@ -46,7 +45,7 @@ static char *cleanup_path(char *path)\n \n static void strbuf_cleanup_path(struct strbuf *sb)\n {\n-\tchar *path = cleanup_path(sb->buf);\n+\tconst char *path = cleanup_path(sb->buf);\n \tif (path > sb->buf)\n \t\tstrbuf_remove(sb, 0, path - sb->buf);\n }\n@@ -63,7 +62,7 @@ char *mksnpath(char *buf, size_t n, const char *fmt, ...)\n \t\tstrlcpy(buf, bad_path, n);\n \t\treturn buf;\n \t}\n-\treturn cleanup_path(buf);\n+\treturn (char *)cleanup_path(buf);\n }\n \n static int dir_prefix(const char *buf, const char *dir)\n-- \n2.14.2-889-gd2948f6aa6\n\n"},{"id":"329645","messageId":"xmqqbmlnecxd.fsf@gitster.mtv.corp.google.com","threadId":"46893","inReplyTo":"20171003233638.fq6lgls2qsucfbn3@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] http-push: fix construction of hex value from path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-04T04:48:14Z","receivedAt":"2017-10-04T04:48:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> Moreover, this is in the webdav-based \"dumb http\" push code path,\n>> which I do not trust much at all.  I wonder if we could retire it\n>> completely (or at least provide an option to turn it off).\n>\n> I would really like that, too. It has been the cause of a lot of pain\n> when working with the smart code, and I am not at all surprised to find\n> a bug of this magnitude lurking in it. I'd _hoped_ this could show that\n> the system has been unusably broken for years, which would give us\n> confidence to turn it off. :) But per your paragraph above, people could\n> very easily still have been happily using it in the meantime.\n\nSame here.  Perhaps we should deliberately and silently break it and\nsee who screams?\n"},{"id":"329646","messageId":"xmqq7ewbecek.fsf@gitster.mtv.corp.google.com","threadId":"46893","inReplyTo":"85456f81-9e9f-d0e4-86f7-9f546691dee3@kdbg.org","subject":"Re: [PATCH 3/3] sub-process: allocate argv on the heap","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-04T04:59:31Z","receivedAt":"2017-10-04T04:59:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n> Am 03.10.2017 um 21:57 schrieb Thomas Gummerer:\n>> diff --git a/sub-process.c b/sub-process.c\n>> index 6dde5062be..4680af8193 100644\n>> --- a/sub-process.c\n>> +++ b/sub-process.c\n>> @@ -77,7 +77,9 @@ int subprocess_start(struct hashmap *hashmap, struct subprocess_entry *entry, co\n>>   {\n>>   \tint err;\n>>   \tstruct child_process *process;\n>> -\tconst char *argv[] = { cmd, NULL };\n>> +\tconst char **argv = xmalloc(2 * sizeof(char *));\n>> +\targv[0] = cmd;\n>> +\targv[1] = NULL;\n>>     \tentry->cmd = cmd;\n>>   \tprocess = &entry->process;\n>>\n>\n> Perhaps this should become\n>\n> \targv_array_push(&process->args, cmd);\n>\n> so that there is no new memory leak?\n\nSounds like a good idea (if I am not grossly mistaken as to what is\nbeing suggested).\n\nHere is what I am planning to queue.\n\n-- >8 --\nFrom: Johannes Sixt <j6t@kdbg.org>\nDate: Tue, 3 Oct 2017 22:24:57 +0200\nSubject: [PATCH] sub-process: use child_process.args instead of child_process.argv\n\nCurrently the argv is only allocated on the stack, and then assigned to\nprocess->argv.  When the start_subprocess function goes out of scope,\nthe local argv variable is eliminated from the stack, but the pointer is\nstill kept around in process->argv.\n\nMuch later when we try to access the same process->argv in\nfinish_command, this leads us to access a memory location that no longer\ncontains what we want.  As argv0 is only used for printing errors, this\nis not easily noticed in normal git operations.  However when running\nt0021-conversion.sh through valgrind, valgrind rightfully complains:\n\n==21024== Invalid read of size 8\n==21024==    at 0x2ACF64: finish_command (run-command.c:869)\n==21024==    by 0x2D6B18: subprocess_exit_handler (sub-process.c:72)\n==21024==    by 0x2AB41E: cleanup_children (run-command.c:45)\n==21024==    by 0x2AB526: cleanup_children_on_exit (run-command.c:81)\n==21024==    by 0x54AD487: __run_exit_handlers (in /usr/lib/libc-2.26.so)\n==21024==    by 0x54AD4D9: exit (in /usr/lib/libc-2.26.so)\n==21024==    by 0x11A9EF: handle_builtin (git.c:550)\n==21024==    by 0x11ABCC: run_argv (git.c:602)\n==21024==    by 0x11AD8E: cmd_main (git.c:679)\n==21024==    by 0x1BF125: main (common-main.c:43)\n==21024==  Address 0x1ffeffec00 is on thread 1's stack\n==21024==  1504 bytes below stack pointer\n==21024==\n\nThese days, the child_process structure has its own args array, and\nthe standard way to set up its argv[] is to use that one, instead of\nassigning to process->argv to point at an array that is outside.\nUse that facility automatically fixes this issue.\n\nReported-by: Thomas Gummerer <t.gummerer@gmail.com>\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n sub-process.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/sub-process.c b/sub-process.c\nindex fcc4832c14..648b3a3943 100644\n--- a/sub-process.c\n+++ b/sub-process.c\n@@ -74,13 +74,12 @@ int subprocess_start(struct hashmap *hashmap, struct subprocess_entry *entry, co\n {\n \tint err;\n \tstruct child_process *process;\n-\tconst char *argv[] = { cmd, NULL };\n \n \tentry->cmd = cmd;\n \tprocess = &entry->process;\n \n \tchild_process_init(process);\n-\tprocess->argv = argv;\n+\targv_array_push(&process->args, cmd);\n \tprocess->use_shell = 1;\n \tprocess->in = -1;\n \tprocess->out = -1;\n-- \n2.14.2-889-gd2948f6aa6\n\n"},{"id":"329648","messageId":"xmqqy3orcwvu.fsf@gitster.mtv.corp.google.com","threadId":"46893","inReplyTo":"xmqqbmlnecxd.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/3] http-push: fix construction of hex value from path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-04T05:20:05Z","receivedAt":"2017-10-04T05:20:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Jeff King <peff@peff.net> writes:\n>\n>>> Moreover, this is in the webdav-based \"dumb http\" push code path,\n>>> which I do not trust much at all.  I wonder if we could retire it\n>>> completely (or at least provide an option to turn it off).\n>>\n>> I would really like that, too. It has been the cause of a lot of pain\n>> when working with the smart code, and I am not at all surprised to find\n>> a bug of this magnitude lurking in it. I'd _hoped_ this could show that\n>> the system has been unusably broken for years, which would give us\n>> confidence to turn it off. :) But per your paragraph above, people could\n>> very easily still have been happily using it in the meantime.\n>\n> Same here.  Perhaps we should deliberately and silently break it and\n> see who screams?\n\nHopefully it should be obvious but just for people with unreasonable\nexpectations, I should clarify that the above needs a smiley ;-).\n"},{"id":"329649","messageId":"20171004052151.4ntqmqe7wdbkbvx7@sigill.intra.peff.net","threadId":"46893","inReplyTo":"xmqqfuazecym.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/3] path.c: fix uninitialized memory access","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-04T05:21:52Z","receivedAt":"2017-10-04T05:21:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 04, 2017 at 01:47:29PM +0900, Junio C Hamano wrote:\n\n> Jonathan Nieder <jrnieder@gmail.com> writes:\n> \n> > Jeff King wrote:\n> >> On Tue, Oct 03, 2017 at 03:45:01PM -0700, Jonathan Nieder wrote:\n> >\n> >>> In other words, an alternative fix would be\n> >>> \n> >>> \tif (*path == '.' && path[1] == '/') {\n> >>> \t\t...\n> >>> \t}\n> >>> \n> >>> which would not require passing in 'len' or switching to index-based\n> >>> arithmetic.  I think I prefer it.  What do you think?\n> >>\n> >> Yes, I think that approach is much nicer. I think you could even use\n> >> skip_prefix. Unfortunately you have to play a few games with const-ness,\n> >> but I think the resulting signature for cleanup_path() is an\n> >> improvement:\n> \n> To tie the loose end, here is what I'll queue.\n\nThank you, I was planning to get to this later tonight, but now I don't\nhave to. :)\n\nFWIW, I wondered if we could get rid of the extra cast by switching\ncleanup_path() to return an offset rather than a string (which also\nmakes its interface more foolproof, since it's clear that the return\nvalue is a subset of the original string).\n\nBut it ends up being a bit clunky I think (patch below for reference).\n\nI guess it's possible that `cleanup_path` could learn to do other\ncleanup, too (e.g., to clean up doubled slashes in the middle of the\npath), in which case it really would want a non-const buffer. But since\nit has remained unchanged since 26c8a533af (Add \"mkpath()\" helper\nfunction, 2005-07-08), I'm happy to assume it will remain so for another\n12 years.\n\nAll of which is to say:\n\n> -- >8 --\n> From: Jeff King <peff@peff.net>\n> Date: Tue, 3 Oct 2017 19:30:40 -0400\n> Subject: [PATCH] path.c: fix uninitialized memory access\n\nLooks good to me.\n\n-Peff\n\n-- >8 --\ndiff --git a/path.c b/path.c\nindex 5aa9244eb2..eaeb9d9a17 100644\n--- a/path.c\n+++ b/path.c\n@@ -34,22 +34,24 @@ static struct strbuf *get_pathname(void)\n \treturn sb;\n }\n \n-static char *cleanup_path(char *path)\n+static size_t cleanup_path(const char *path)\n {\n-\t/* Clean it up */\n-\tif (!memcmp(path, \"./\", 2)) {\n-\t\tpath += 2;\n-\t\twhile (*path == '/')\n-\t\t\tpath++;\n-\t}\n-\treturn path;\n+\tconst char *clean;\n+\n+\tif (!skip_prefix(path, \"./\", &clean))\n+\t\treturn 0;\n+\n+\twhile (*clean == '/')\n+\t\tclean++;\n+\n+\treturn clean - path;\n }\n \n static void strbuf_cleanup_path(struct strbuf *sb)\n {\n-\tchar *path = cleanup_path(sb->buf);\n-\tif (path > sb->buf)\n-\t\tstrbuf_remove(sb, 0, path - sb->buf);\n+\tsize_t s = cleanup_path(sb->buf);\n+\tif (s)\n+\t\tstrbuf_remove(sb, 0, s);\n }\n \n char *mksnpath(char *buf, size_t n, const char *fmt, ...)\n@@ -64,7 +66,7 @@ char *mksnpath(char *buf, size_t n, const char *fmt, ...)\n \t\tstrlcpy(buf, bad_path, n);\n \t\treturn buf;\n \t}\n-\treturn cleanup_path(buf);\n+\treturn buf + cleanup_path(buf);\n }\n \n static int dir_prefix(const char *buf, const char *dir)\n"},{"id":"329650","messageId":"20171004052640.3sxpluokhcyh4c2y@sigill.intra.peff.net","threadId":"46893","inReplyTo":"xmqqy3orcwvu.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/3] http-push: fix construction of hex value from path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-04T05:26:40Z","receivedAt":"2017-10-04T05:26:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 04, 2017 at 02:20:05PM +0900, Junio C Hamano wrote:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > Jeff King <peff@peff.net> writes:\n> >\n> >>> Moreover, this is in the webdav-based \"dumb http\" push code path,\n> >>> which I do not trust much at all.  I wonder if we could retire it\n> >>> completely (or at least provide an option to turn it off).\n> >>\n> >> I would really like that, too. It has been the cause of a lot of pain\n> >> when working with the smart code, and I am not at all surprised to find\n> >> a bug of this magnitude lurking in it. I'd _hoped_ this could show that\n> >> the system has been unusably broken for years, which would give us\n> >> confidence to turn it off. :) But per your paragraph above, people could\n> >> very easily still have been happily using it in the meantime.\n> >\n> > Same here.  Perhaps we should deliberately and silently break it and\n> > see who screams?\n> \n> Hopefully it should be obvious but just for people with unreasonable\n> expectations, I should clarify that the above needs a smiley ;-).\n\nYes, I was surprised to see it without one. :)\n\nMore seriously, is there any interest in marking it as deprecated in the\nrelease notes and issuing a warning when it's used for a few cycles?\n\n-Peff\n"},{"id":"329652","messageId":"20171004053208.6vha2qj23md6t44y@sigill.intra.peff.net","threadId":"46893","inReplyTo":"xmqq7ewbecek.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/3] sub-process: allocate argv on the heap","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-04T05:32:08Z","receivedAt":"2017-10-04T05:32:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 04, 2017 at 01:59:31PM +0900, Junio C Hamano wrote:\n\n> > Perhaps this should become\n> >\n> > \targv_array_push(&process->args, cmd);\n> >\n> > so that there is no new memory leak?\n> \n> Sounds like a good idea (if I am not grossly mistaken as to what is\n> being suggested).\n> \n> Here is what I am planning to queue.\n> \n> -- >8 --\n> From: Johannes Sixt <j6t@kdbg.org>\n> Date: Tue, 3 Oct 2017 22:24:57 +0200\n> Subject: [PATCH] sub-process: use child_process.args instead of child_process.argv\n\nThis looks good (and is exactly the type of case for which I added\n\"args\" to the child_process in the first place). The commit message\nis well-explained and the patch looks obviously correct.\n\n-Peff\n"},{"id":"329653","messageId":"d25458d1-6244-93b0-367d-c2a4efb9d267@kdbg.org","threadId":"46893","inReplyTo":"xmqq7ewbecek.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/3] sub-process: allocate argv on the heap","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2017-10-04T05:58:14Z","receivedAt":"2017-10-04T05:58:21Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 04.10.2017 um 06:59 schrieb Junio C Hamano:\n> Johannes Sixt <j6t@kdbg.org> writes:\n> \n>> Am 03.10.2017 um 21:57 schrieb Thomas Gummerer:\n>>> diff --git a/sub-process.c b/sub-process.c\n>>> index 6dde5062be..4680af8193 100644\n>>> --- a/sub-process.c\n>>> +++ b/sub-process.c\n>>> @@ -77,7 +77,9 @@ int subprocess_start(struct hashmap *hashmap, struct subprocess_entry *entry, co\n>>>    {\n>>>    \tint err;\n>>>    \tstruct child_process *process;\n>>> -\tconst char *argv[] = { cmd, NULL };\n>>> +\tconst char **argv = xmalloc(2 * sizeof(char *));\n>>> +\targv[0] = cmd;\n>>> +\targv[1] = NULL;\n>>>      \tentry->cmd = cmd;\n>>>    \tprocess = &entry->process;\n>>>\n>>\n>> Perhaps this should become\n>>\n>> \targv_array_push(&process->args, cmd);\n>>\n>> so that there is no new memory leak?\n> \n> Sounds like a good idea (if I am not grossly mistaken as to what is\n> being suggested).\n> \n> Here is what I am planning to queue.\n> \n> -- >8 --\n> From: Johannes Sixt <j6t@kdbg.org>\n> Date: Tue, 3 Oct 2017 22:24:57 +0200\n> Subject: [PATCH] sub-process: use child_process.args instead of child_process.argv\n> \n> Currently the argv is only allocated on the stack, and then assigned to\n> process->argv.  When the start_subprocess function goes out of scope,\n> the local argv variable is eliminated from the stack, but the pointer is\n> still kept around in process->argv.\n> \n> Much later when we try to access the same process->argv in\n> finish_command, this leads us to access a memory location that no longer\n> contains what we want.  As argv0 is only used for printing errors, this\n> is not easily noticed in normal git operations.  However when running\n> t0021-conversion.sh through valgrind, valgrind rightfully complains:\n> \n> ==21024== Invalid read of size 8\n> ==21024==    at 0x2ACF64: finish_command (run-command.c:869)\n> ==21024==    by 0x2D6B18: subprocess_exit_handler (sub-process.c:72)\n> ==21024==    by 0x2AB41E: cleanup_children (run-command.c:45)\n> ==21024==    by 0x2AB526: cleanup_children_on_exit (run-command.c:81)\n> ==21024==    by 0x54AD487: __run_exit_handlers (in /usr/lib/libc-2.26.so)\n> ==21024==    by 0x54AD4D9: exit (in /usr/lib/libc-2.26.so)\n> ==21024==    by 0x11A9EF: handle_builtin (git.c:550)\n> ==21024==    by 0x11ABCC: run_argv (git.c:602)\n> ==21024==    by 0x11AD8E: cmd_main (git.c:679)\n> ==21024==    by 0x1BF125: main (common-main.c:43)\n> ==21024==  Address 0x1ffeffec00 is on thread 1's stack\n> ==21024==  1504 bytes below stack pointer\n> ==21024==\n> \n> These days, the child_process structure has its own args array, and\n> the standard way to set up its argv[] is to use that one, instead of\n> assigning to process->argv to point at an array that is outside.\n> Use that facility automatically fixes this issue.\n> \n> Reported-by: Thomas Gummerer <t.gummerer@gmail.com>\n> Signed-off-by: Johannes Sixt <j6t@kdbg.org>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>   sub-process.c | 3 +--\n>   1 file changed, 1 insertion(+), 2 deletions(-)\n> \n> diff --git a/sub-process.c b/sub-process.c\n> index fcc4832c14..648b3a3943 100644\n> --- a/sub-process.c\n> +++ b/sub-process.c\n> @@ -74,13 +74,12 @@ int subprocess_start(struct hashmap *hashmap, struct subprocess_entry *entry, co\n>   {\n>   \tint err;\n>   \tstruct child_process *process;\n> -\tconst char *argv[] = { cmd, NULL };\n>   \n>   \tentry->cmd = cmd;\n>   \tprocess = &entry->process;\n>   \n>   \tchild_process_init(process);\n> -\tprocess->argv = argv;\n> +\targv_array_push(&process->args, cmd);\n>   \tprocess->use_shell = 1;\n>   \tprocess->in = -1;\n>   \tprocess->out = -1;\n> \n\nThank you very much! That looks good. Just to be on the safe side:\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n\n-- Hannes\n"},{"id":"329658","messageId":"xmqqd163cttl.fsf@gitster.mtv.corp.google.com","threadId":"46893","inReplyTo":"20171004052640.3sxpluokhcyh4c2y@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] http-push: fix construction of hex value from path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-04T06:26:14Z","receivedAt":"2017-10-04T06:26:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> More seriously, is there any interest in marking it as deprecated in the\n> release notes and issuing a warning when it's used for a few cycles?\n\nNo objection from me.\n\nI do not object with such a well designed deprecation plan for any\nother features, if there are other \"favourite\" ones people would\nwant to deprecate and eventually remove, by the way.\n"},{"id":"329674","messageId":"20171004101932.pai6wzcv2eohsicr@sigill.intra.peff.net","threadId":"46893","inReplyTo":"20171003234153.pmslizidgbifl6en@sigill.intra.peff.net","subject":"playing with MSan, was Re: [PATCH 0/3] fixes for running the test suite with --valgrind","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-04T10:19:32Z","receivedAt":"2017-10-04T10:19:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 03, 2017 at 07:41:54PM -0400, Jeff King wrote:\n\n> I think using SANITIZE=memory would catch these, but it needs some\n> suppressions tuning. The weird \"zlib reads uninitialized memory\" error\n> is a problem (valgrind sees this, too, but we have suppressions).\n\nI dug into this a little more. You can blacklist certain functions from\ngetting MSan treatment, but that's not quite what we want. We want to\nmark bytes from certain _sources_ as being initialized, even if MSan\ndoesn't agree.\n\nAnd indeed, you can do that. As far as I can tell, MSan works by keeping\na shadow map of memory and setting flags when it believes it has been\ninitialized, and then checking that map when we make decisions based on\nthe memory. But it can only do that if it instruments all writes. So the\nMSan documentation recommends that you build _everything_, including\nlibraries, with it. Which obviously we don't do if we're using a system\nzlib. Or a system libc for that matter (though they intercept many\ncommon libc functions to handle this).\n\nSo one strategy is to \"cheat\" a bit at the library interfaces, and claim\nwhatever they send us is properly initialized. The patch below tries\nthat with zlib, and it does seem to work. It would fail to notice a real\nproblem with any input we send _to_ the library (since the library isn't\ninstrumented, and we claim that whatever comes out of it is legitimate).\nI could probably live with that.\n\nBut there are quite a few test failures that would still need\ninvestigating and annotating:\n\n  - Certainly it's confused by looking at regmatch_t results from\n    regexec(). We can fix that by building with NO_REGEX. But pcre has\n    a similar problem.\n\n  - Ditto curl and openssl, whose exit points would need annotations.\n\n  - For some reason test-sigchain segfaults when it raise()s in the\n    signal handler and recurses. Not sure if this is an MSan bug or\n    what.\n\nSo I dunno. This approach is a _lot_ more convenient than trying to\nrebuild all the dependencies from scratch, and it runs way faster than\nvalgrind. It did find the cases that led to the patches in this\nseries, and at least one more: if the lstat() at the end of\nentry.c:write_entry() fails, we write nonsense into the cache_entry.\n\nI think we could probably get it to zero false positives without _too_\nmuch effort. I'll stop here for tonight, but I may pick it up again\nlater (of course anybody else is welcome to fool around with it, too).\n\nBelow is the patch that let me run:\n\n  make SANITIZE=memory CC=clang-6.0 NO_REGEX=1\n\nand get a tractable number of errors.\n\n-- >8 --\ndiff --git a/Makefile b/Makefile\nindex b143e4eea3..1da5c01211 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1047,6 +1047,9 @@ endif\n ifneq ($(filter leak,$(SANITIZERS)),)\n BASIC_CFLAGS += -DSUPPRESS_ANNOTATED_LEAKS\n endif\n+ifneq ($(filter memory,$(SANITIZERS)),)\n+BASIC_CFLAGS += -DENABLE_MSAN_UNPOISON\n+endif\n endif\n \n ifndef sysconfdir\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex cedad4d581..836a4c0b54 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -1191,4 +1191,11 @@ extern void unleak_memory(const void *ptr, size_t len);\n #define UNLEAK(var) do {} while (0)\n #endif\n \n+#ifdef ENABLE_MSAN_UNPOISON\n+#include <sanitizer/msan_interface.h>\n+#define msan_unpoison(ptr, len) __msan_unpoison(ptr, len)\n+#else\n+#define msan_unpoison(ptr, len) do {} while (0)\n+#endif\n+\n #endif\ndiff --git a/zlib.c b/zlib.c\nindex 4223f1a8c5..5fa8f12507 100644\n--- a/zlib.c\n+++ b/zlib.c\n@@ -56,6 +56,8 @@ static void zlib_post_call(git_zstream *s)\n \tif (s->z.total_in != s->total_in + bytes_consumed)\n \t\tdie(\"BUG: total_in mismatch\");\n \n+\tmsan_unpoison(s->next_out, bytes_produced);\n+\n \ts->total_out = s->z.total_out;\n \ts->total_in = s->z.total_in;\n \ts->next_in = s->z.next_in;\n"},{"id":"329703","messageId":"20171004192243.GC30301@hank","threadId":"46893","inReplyTo":"xmqqfuazecym.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/3] path.c: fix uninitialized memory access","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2017-10-04T19:22:43Z","receivedAt":"2017-10-04T19:22:10Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 10/04, Junio C Hamano wrote:\n> Jonathan Nieder <jrnieder@gmail.com> writes:\n> \n> > Jeff King wrote:\n> >> On Tue, Oct 03, 2017 at 03:45:01PM -0700, Jonathan Nieder wrote:\n> >\n> >>> In other words, an alternative fix would be\n> >>> \n> >>> \tif (*path == '.' && path[1] == '/') {\n> >>> \t\t...\n> >>> \t}\n> >>> \n> >>> which would not require passing in 'len' or switching to index-based\n> >>> arithmetic.  I think I prefer it.  What do you think?\n> >>\n> >> Yes, I think that approach is much nicer. I think you could even use\n> >> skip_prefix. Unfortunately you have to play a few games with const-ness,\n> >> but I think the resulting signature for cleanup_path() is an\n> >> improvement:\n> \n> To tie the loose end, here is what I'll queue.\n\nThanks.  This is much nicer indeed!\n\n> -- >8 --\n> From: Jeff King <peff@peff.net>\n> Date: Tue, 3 Oct 2017 19:30:40 -0400\n> Subject: [PATCH] path.c: fix uninitialized memory access\n> \n> In cleanup_path we're passing in a char array, run a memcmp on it, and\n> run through it without ever checking if something is in the array in the\n> first place.  This can lead us to access uninitialized memory, for\n> example in t5541-http-push-smart.sh test 7, when run under valgrind:\n> \n> ==4423== Conditional jump or move depends on uninitialised value(s)\n> ==4423==    at 0x242FA9: cleanup_path (path.c:35)\n> ==4423==    by 0x242FA9: mkpath (path.c:456)\n> ==4423==    by 0x256CC7: refname_match (refs.c:364)\n> ==4423==    by 0x26C181: count_refspec_match (remote.c:1015)\n> ==4423==    by 0x26C181: match_explicit_lhs (remote.c:1126)\n> ==4423==    by 0x26C181: check_push_refs (remote.c:1409)\n> ==4423==    by 0x2ABB4D: transport_push (transport.c:870)\n> ==4423==    by 0x186703: push_with_options (push.c:332)\n> ==4423==    by 0x18746D: do_push (push.c:409)\n> ==4423==    by 0x18746D: cmd_push (push.c:566)\n> ==4423==    by 0x1183E0: run_builtin (git.c:352)\n> ==4423==    by 0x11973E: handle_builtin (git.c:539)\n> ==4423==    by 0x11973E: run_argv (git.c:593)\n> ==4423==    by 0x11973E: main (git.c:698)\n> ==4423==  Uninitialised value was created by a heap allocation\n> ==4423==    at 0x4C2CD8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)\n> ==4423==    by 0x4C2F195: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)\n> ==4423==    by 0x2C196B: xrealloc (wrapper.c:137)\n> ==4423==    by 0x29A30B: strbuf_grow (strbuf.c:66)\n> ==4423==    by 0x29A30B: strbuf_vaddf (strbuf.c:277)\n> ==4423==    by 0x242F9F: mkpath (path.c:454)\n> ==4423==    by 0x256CC7: refname_match (refs.c:364)\n> ==4423==    by 0x26C181: count_refspec_match (remote.c:1015)\n> ==4423==    by 0x26C181: match_explicit_lhs (remote.c:1126)\n> ==4423==    by 0x26C181: check_push_refs (remote.c:1409)\n> ==4423==    by 0x2ABB4D: transport_push (transport.c:870)\n> ==4423==    by 0x186703: push_with_options (push.c:332)\n> ==4423==    by 0x18746D: do_push (push.c:409)\n> ==4423==    by 0x18746D: cmd_push (push.c:566)\n> ==4423==    by 0x1183E0: run_builtin (git.c:352)\n> ==4423==    by 0x11973E: handle_builtin (git.c:539)\n> ==4423==    by 0x11973E: run_argv (git.c:593)\n> ==4423==    by 0x11973E: main (git.c:698)\n> ==4423==\n> \n> Avoid this by using skip_prefix(), which knows not to go beyond the\n> end of the string.\n> \n> Reported-by: Thomas Gummerer <t.gummerer@gmail.com>\n> Signed-off-by: Jeff King <peff@peff.net>\n> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  path.c | 9 ++++-----\n>  1 file changed, 4 insertions(+), 5 deletions(-)\n> \n> diff --git a/path.c b/path.c\n> index e50d2befcf..2fecf854fe 100644\n> --- a/path.c\n> +++ b/path.c\n> @@ -33,11 +33,10 @@ static struct strbuf *get_pathname(void)\n>  \treturn sb;\n>  }\n>  \n> -static char *cleanup_path(char *path)\n> +static const char *cleanup_path(const char *path)\n>  {\n>  \t/* Clean it up */\n> -\tif (!memcmp(path, \"./\", 2)) {\n> -\t\tpath += 2;\n> +\tif (skip_prefix(path, \"./\", &path)) {\n>  \t\twhile (*path == '/')\n>  \t\t\tpath++;\n>  \t}\n> @@ -46,7 +45,7 @@ static char *cleanup_path(char *path)\n>  \n>  static void strbuf_cleanup_path(struct strbuf *sb)\n>  {\n> -\tchar *path = cleanup_path(sb->buf);\n> +\tconst char *path = cleanup_path(sb->buf);\n>  \tif (path > sb->buf)\n>  \t\tstrbuf_remove(sb, 0, path - sb->buf);\n>  }\n> @@ -63,7 +62,7 @@ char *mksnpath(char *buf, size_t n, const char *fmt, ...)\n>  \t\tstrlcpy(buf, bad_path, n);\n>  \t\treturn buf;\n>  \t}\n> -\treturn cleanup_path(buf);\n> +\treturn (char *)cleanup_path(buf);\n>  }\n>  \n>  static int dir_prefix(const char *buf, const char *dir)\n> -- \n> 2.14.2-889-gd2948f6aa6\n> \n"},{"id":"329704","messageId":"20171004193005.GD30301@hank","threadId":"46893","inReplyTo":"20171004101932.pai6wzcv2eohsicr@sigill.intra.peff.net","subject":"Re: playing with MSan, was Re: [PATCH 0/3] fixes for running the test suite with --valgrind","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2017-10-04T19:30:05Z","receivedAt":"2017-10-04T19:29:21Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 10/04, Jeff King wrote:\n> On Tue, Oct 03, 2017 at 07:41:54PM -0400, Jeff King wrote:\n> \n> > I think using SANITIZE=memory would catch these, but it needs some\n> > suppressions tuning. The weird \"zlib reads uninitialized memory\" error\n> > is a problem (valgrind sees this, too, but we have suppressions).\n> \n> I dug into this a little more. You can blacklist certain functions from\n> getting MSan treatment, but that's not quite what we want. We want to\n> mark bytes from certain _sources_ as being initialized, even if MSan\n> doesn't agree.\n> \n> And indeed, you can do that. As far as I can tell, MSan works by keeping\n> a shadow map of memory and setting flags when it believes it has been\n> initialized, and then checking that map when we make decisions based on\n> the memory. But it can only do that if it instruments all writes. So the\n> MSan documentation recommends that you build _everything_, including\n> libraries, with it. Which obviously we don't do if we're using a system\n> zlib. Or a system libc for that matter (though they intercept many\n> common libc functions to handle this).\n> \n> So one strategy is to \"cheat\" a bit at the library interfaces, and claim\n> whatever they send us is properly initialized. The patch below tries\n> that with zlib, and it does seem to work. It would fail to notice a real\n> problem with any input we send _to_ the library (since the library isn't\n> instrumented, and we claim that whatever comes out of it is legitimate).\n> I could probably live with that.\n> \n> But there are quite a few test failures that would still need\n> investigating and annotating:\n> \n>   - Certainly it's confused by looking at regmatch_t results from\n>     regexec(). We can fix that by building with NO_REGEX. But pcre has\n>     a similar problem.\n> \n>   - Ditto curl and openssl, whose exit points would need annotations.\n> \n>   - For some reason test-sigchain segfaults when it raise()s in the\n>     signal handler and recurses. Not sure if this is an MSan bug or\n>     what.\n> \n> So I dunno. This approach is a _lot_ more convenient than trying to\n> rebuild all the dependencies from scratch, and it runs way faster than\n> valgrind.  It did find the cases that led to the patches in this\n> series, and at least one more: if the lstat() at the end of\n> entry.c:write_entry() fails, we write nonsense into the cache_entry.\n\nYeah valgrind found that one too, as I tried (and apparently failed :))\nto explain in the cover letter.  I just haven't found the time yet to\nactually try and go fix that one.\n\n> I think we could probably get it to zero false positives without _too_\n> much effort. I'll stop here for tonight, but I may pick it up again\n> later (of course anybody else is welcome to fool around with it, too).\n> \n> Below is the patch that let me run:\n> \n>   make SANITIZE=memory CC=clang-6.0 NO_REGEX=1\n> \n> and get a tractable number of errors.\n> \n> -- >8 --\n> diff --git a/Makefile b/Makefile\n> index b143e4eea3..1da5c01211 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -1047,6 +1047,9 @@ endif\n>  ifneq ($(filter leak,$(SANITIZERS)),)\n>  BASIC_CFLAGS += -DSUPPRESS_ANNOTATED_LEAKS\n>  endif\n> +ifneq ($(filter memory,$(SANITIZERS)),)\n> +BASIC_CFLAGS += -DENABLE_MSAN_UNPOISON\n> +endif\n>  endif\n>  \n>  ifndef sysconfdir\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index cedad4d581..836a4c0b54 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -1191,4 +1191,11 @@ extern void unleak_memory(const void *ptr, size_t len);\n>  #define UNLEAK(var) do {} while (0)\n>  #endif\n>  \n> +#ifdef ENABLE_MSAN_UNPOISON\n> +#include <sanitizer/msan_interface.h>\n> +#define msan_unpoison(ptr, len) __msan_unpoison(ptr, len)\n> +#else\n> +#define msan_unpoison(ptr, len) do {} while (0)\n> +#endif\n> +\n>  #endif\n> diff --git a/zlib.c b/zlib.c\n> index 4223f1a8c5..5fa8f12507 100644\n> --- a/zlib.c\n> +++ b/zlib.c\n> @@ -56,6 +56,8 @@ static void zlib_post_call(git_zstream *s)\n>  \tif (s->z.total_in != s->total_in + bytes_consumed)\n>  \t\tdie(\"BUG: total_in mismatch\");\n>  \n> +\tmsan_unpoison(s->next_out, bytes_produced);\n> +\n>  \ts->total_out = s->z.total_out;\n>  \ts->total_in = s->z.total_in;\n>  \ts->next_in = s->z.next_in;\n"},{"id":"329705","messageId":"20171004193107.GE30301@hank","threadId":"46893","inReplyTo":"xmqq7ewbecek.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/3] sub-process: allocate argv on the heap","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2017-10-04T19:31:07Z","receivedAt":"2017-10-04T19:30:22Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 10/04, Junio C Hamano wrote:\n> Johannes Sixt <j6t@kdbg.org> writes:\n> \n> > Am 03.10.2017 um 21:57 schrieb Thomas Gummerer:\n> >> diff --git a/sub-process.c b/sub-process.c\n> >> index 6dde5062be..4680af8193 100644\n> >> --- a/sub-process.c\n> >> +++ b/sub-process.c\n> >> @@ -77,7 +77,9 @@ int subprocess_start(struct hashmap *hashmap, struct subprocess_entry *entry, co\n> >>   {\n> >>   \tint err;\n> >>   \tstruct child_process *process;\n> >> -\tconst char *argv[] = { cmd, NULL };\n> >> +\tconst char **argv = xmalloc(2 * sizeof(char *));\n> >> +\targv[0] = cmd;\n> >> +\targv[1] = NULL;\n> >>     \tentry->cmd = cmd;\n> >>   \tprocess = &entry->process;\n> >>\n> >\n> > Perhaps this should become\n> >\n> > \targv_array_push(&process->args, cmd);\n> >\n> > so that there is no new memory leak?\n> \n> Sounds like a good idea (if I am not grossly mistaken as to what is\n> being suggested).\n> \n> Here is what I am planning to queue.\n> \n> -- >8 --\n> From: Johannes Sixt <j6t@kdbg.org>\n> Date: Tue, 3 Oct 2017 22:24:57 +0200\n> Subject: [PATCH] sub-process: use child_process.args instead of child_process.argv\n> \n> Currently the argv is only allocated on the stack, and then assigned to\n> process->argv.  When the start_subprocess function goes out of scope,\n> the local argv variable is eliminated from the stack, but the pointer is\n> still kept around in process->argv.\n> \n> Much later when we try to access the same process->argv in\n> finish_command, this leads us to access a memory location that no longer\n> contains what we want.  As argv0 is only used for printing errors, this\n> is not easily noticed in normal git operations.  However when running\n> t0021-conversion.sh through valgrind, valgrind rightfully complains:\n> \n> ==21024== Invalid read of size 8\n> ==21024==    at 0x2ACF64: finish_command (run-command.c:869)\n> ==21024==    by 0x2D6B18: subprocess_exit_handler (sub-process.c:72)\n> ==21024==    by 0x2AB41E: cleanup_children (run-command.c:45)\n> ==21024==    by 0x2AB526: cleanup_children_on_exit (run-command.c:81)\n> ==21024==    by 0x54AD487: __run_exit_handlers (in /usr/lib/libc-2.26.so)\n> ==21024==    by 0x54AD4D9: exit (in /usr/lib/libc-2.26.so)\n> ==21024==    by 0x11A9EF: handle_builtin (git.c:550)\n> ==21024==    by 0x11ABCC: run_argv (git.c:602)\n> ==21024==    by 0x11AD8E: cmd_main (git.c:679)\n> ==21024==    by 0x1BF125: main (common-main.c:43)\n> ==21024==  Address 0x1ffeffec00 is on thread 1's stack\n> ==21024==  1504 bytes below stack pointer\n> ==21024==\n> \n> These days, the child_process structure has its own args array, and\n> the standard way to set up its argv[] is to use that one, instead of\n> assigning to process->argv to point at an array that is outside.\n> Use that facility automatically fixes this issue.\n> \n> Reported-by: Thomas Gummerer <t.gummerer@gmail.com>\n> Signed-off-by: Johannes Sixt <j6t@kdbg.org>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  sub-process.c | 3 +--\n>  1 file changed, 1 insertion(+), 2 deletions(-)\n> \n> diff --git a/sub-process.c b/sub-process.c\n> index fcc4832c14..648b3a3943 100644\n> --- a/sub-process.c\n> +++ b/sub-process.c\n> @@ -74,13 +74,12 @@ int subprocess_start(struct hashmap *hashmap, struct subprocess_entry *entry, co\n>  {\n>  \tint err;\n>  \tstruct child_process *process;\n> -\tconst char *argv[] = { cmd, NULL };\n>  \n>  \tentry->cmd = cmd;\n>  \tprocess = &entry->process;\n>  \n>  \tchild_process_init(process);\n> -\tprocess->argv = argv;\n> +\targv_array_push(&process->args, cmd);\n\nThanks!  *Much* nicer than what I had :)\n\n>  \tprocess->use_shell = 1;\n>  \tprocess->in = -1;\n>  \tprocess->out = -1;\n> -- \n> 2.14.2-889-gd2948f6aa6\n> \n"},{"id":"329706","messageId":"20171004193617.GR19555@aiede.mtv.corp.google.com","threadId":"46893","inReplyTo":"xmqqfuazecym.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/3] path.c: fix uninitialized memory access","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-10-04T19:36:17Z","receivedAt":"2017-10-04T19:36:26Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> From: Jeff King <peff@peff.net>\n> Date: Tue, 3 Oct 2017 19:30:40 -0400\n> Subject: [PATCH] path.c: fix uninitialized memory access\n> \n> In cleanup_path we're passing in a char array, run a memcmp on it, and\n> run through it without ever checking if something is in the array in the\n> first place.  This can lead us to access uninitialized memory, for\n> example in t5541-http-push-smart.sh test 7, when run under valgrind:\n>\n> ==4423== Conditional jump or move depends on uninitialised value(s)\n> ==4423==    at 0x242FA9: cleanup_path (path.c:35)\n[...]\n> ==4423==  Uninitialised value was created by a heap allocation\n[...]\n> ==4423==    by 0x29A30B: strbuf_grow (strbuf.c:66)\n> ==4423==    by 0x29A30B: strbuf_vaddf (strbuf.c:277)\n> ==4423==    by 0x242F9F: mkpath (path.c:454)\n[...]\n> Avoid this by using skip_prefix(), which knows not to go beyond the\n> end of the string.\n>\n> Reported-by: Thomas Gummerer <t.gummerer@gmail.com>\n> Signed-off-by: Jeff King <peff@peff.net>\n> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThis is indeed\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThanks.\n"},{"id":"329721","messageId":"20171005034632.kzsspk7wsuk23kf2@sigill.intra.peff.net","threadId":"46893","inReplyTo":"20171004193005.GD30301@hank","subject":"lstat-ing delayed-filter output, was Re: playing with MSan","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-05T03:46:32Z","receivedAt":"2017-10-05T03:46:38Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 04, 2017 at 08:30:05PM +0100, Thomas Gummerer wrote:\n\n> > So I dunno. This approach is a _lot_ more convenient than trying to\n> > rebuild all the dependencies from scratch, and it runs way faster than\n> > valgrind.  It did find the cases that led to the patches in this\n> > series, and at least one more: if the lstat() at the end of\n> > entry.c:write_entry() fails, we write nonsense into the cache_entry.\n> \n> Yeah valgrind found that one too, as I tried (and apparently failed :))\n> to explain in the cover letter.  I just haven't found the time yet to\n> actually try and go fix that one.\n\nNo, I just have poor memory. :)\n\nThe obvious fix is that we should check the return value of `lstat`, but\nthe bigger question is why and when that would fail.\n\nThe case triggered by t0021 is using the new \"delayed\" filter mechanism.\nSo at the time that write_entry() finishes, we don't actually have the\nfile in the filesystem. I think we need to recognize that we got delayed\nand didn't actually check anything out, and skip that whole \"if\n(state->refresh_cache)\" block. It's not clear to me, though, how we tell\nthe difference between the delayed and normal cases in that function.\n\nBut I think this lstat could also fail if we are checking out and\nsomebody else racily deletes our file. This is presumably sufficiently\nrare that I actually wonder if we should just bail with an error, so\nthat the user knows something funny is going on.\n\n+cc Lars for thoughts no the delayed-filter case.\n\n-Peff\n"},{"id":"329749","messageId":"5BC2435F-B7CD-4B82-B5BD-69B4195EF728@gmail.com","threadId":"46893","inReplyTo":"20171005034632.kzsspk7wsuk23kf2@sigill.intra.peff.net","subject":"Re: lstat-ing delayed-filter output, was Re: playing with MSan","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-10-05T10:47:13Z","receivedAt":"2017-10-05T10:47:22Z","isPatch":false,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 05 Oct 2017, at 05:46, Jeff King <peff@peff.net> wrote:\n> \n> On Wed, Oct 04, 2017 at 08:30:05PM +0100, Thomas Gummerer wrote:\n> \n>>> So I dunno. This approach is a _lot_ more convenient than trying to\n>>> rebuild all the dependencies from scratch, and it runs way faster than\n>>> valgrind.  It did find the cases that led to the patches in this\n>>> series, and at least one more: if the lstat() at the end of\n>>> entry.c:write_entry() fails, we write nonsense into the cache_entry.\n>> \n>> Yeah valgrind found that one too, as I tried (and apparently failed :))\n>> to explain in the cover letter.  I just haven't found the time yet to\n>> actually try and go fix that one.\n> \n> No, I just have poor memory. :)\n> \n> The obvious fix is that we should check the return value of `lstat`, but\n> the bigger question is why and when that would fail.\n> \n> The case triggered by t0021 is using the new \"delayed\" filter mechanism.\n> So at the time that write_entry() finishes, we don't actually have the\n> file in the filesystem. I think we need to recognize that we got delayed\n> and didn't actually check anything out, and skip that whole \"if\n> (state->refresh_cache)\" block. It's not clear to me, though, how we tell\n> the difference between the delayed and normal cases in that function.\n\nOh. Great catch!\n\n\n> But I think this lstat could also fail if we are checking out and\n> somebody else racily deletes our file. This is presumably sufficiently\n> rare that I actually wonder if we should just bail with an error, so\n> that the user knows something funny is going on.\n\nThat sounds good to me!\n\n\nI tried to address both issues here:\nhttps://public-inbox.org/git/20171005104407.65948-1-lars.schneider@autodesk.com/\n\nCheers,\nLars"}]}