{"thread":{"id":"22275","subject":"[PATCH] grep: don't segfault","startedAt":"2010-01-18T21:55:07Z","lastAt":"2010-01-18T22:40:07Z","messageCount":2,"participants":["Jim Meyering","Linus Torvalds"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"132053","messageId":"871vhnhydg.fsf@meyering.net","threadId":"22275","inReplyTo":null,"subject":"[PATCH] grep: don't segfault","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2010-01-18T21:55:07Z","receivedAt":"2010-01-18T21:55:07Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"\nToday, git grep -l '^\t' failed with a segfault.\nI tracked it down to a26345b6085340ebd61e156aa8154a80196bee0f.\nDebugging it, I found that look_ahead calls regexec with a \"bol\"\nargument pointing to a buffer that is not NUL-terminated.\nYet regexec requires the NUL-termination.  Without that, its\ninitial strlen will read beyond the end of the buffer.\n\n    $ valgrind ./git grep -l '^  '\n    Memcheck, a memory error detector\n    Copyright (C) 2002-2009, and GNU GPL'd, by Julian Seward et al.\n    Using Valgrind-3.5.0 and LibVEX; rerun with -h for copyright info\n    Command: ./git grep -l ^\n\n    Conditional jump or move depends on uninitialised value(s)\n       at 0x4A06318: __GI_strlen (mc_replace_strmem.c:284)\n       by 0x3AF00C04C4: regexec@@GLIBC_2.3.4 (regexec.c:243)\n       by 0x496A0A: look_ahead (grep.c:644)\n       by 0x496D24: grep_buffer_1 (grep.c:726)\n       by 0x497161: grep_buffer (grep.c:852)\n       by 0x432720: grep_file (builtin-grep.c:201)\n       by 0x432854: grep_cache (builtin-grep.c:230)\n       by 0x433F2F: cmd_grep (builtin-grep.c:621)\n       by 0x40488D: run_builtin (git.c:257)\n       by 0x404A28: handle_internal_command (git.c:401)\n       by 0x404B1D: run_argv (git.c:443)\n       by 0x404C9A: main (git.c:514)\n    ...\n\nThat buffer is read by builtin-grep.c(grep_file) and here's the code.\nYou can see from the allocation of \"sz + 1\" and read of one less\nthat there is already room for the required trailing NUL byte:\n\n\tsz = xsize_t(st.st_size);\n\ti = open(filename, O_RDONLY);\n\tif (i < 0)\n\t\tgoto err_ret;\n\tdata = xmalloc(sz + 1);\n\tif (st.st_size != read_in_full(i, data, sz)) {\n\t\terror(\"'%s': short read %s\", filename, strerror(errno));\n\t\tclose(i);\n\t\tfree(data);\n\t\treturn 0;\n\t}\n\tclose(i);\n>>>     data[sz] = 0;    <======  added line\n\tif (opt->relative && opt->prefix_length)\n\t\tfilename = quote_path_relative(filename, -1, &buf, opt->prefix);\n\ti = grep_buffer(opt, filename, data, sz);\n\nSigned-off-by: Jim Meyering <meyering@redhat.com>\n---\nThis is relative to the head of git's next branch,\nd7346b15ca5bda881f5abc37b0844e9b35c8cffc\nI noticed the segfault by running the same command in\nlibvirt's git repository, but it's not consistently reproducible.\n\n builtin-grep.c |    1 +\n 1 files changed, 1 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-grep.c b/builtin-grep.c\nindex 4dd5aaf..da854fa 100644\n--- a/builtin-grep.c\n+++ b/builtin-grep.c\n@@ -196,6 +196,7 @@ static int grep_file(struct grep_opt *opt, const char *filename)\n \t\treturn 0;\n \t}\n \tclose(i);\n+\tdata[sz] = 0;\n \tif (opt->relative && opt->prefix_length)\n \t\tfilename = quote_path_relative(filename, -1, &buf, opt->prefix);\n \ti = grep_buffer(opt, filename, data, sz);\n--\n1.6.4.4.1.ga2634\n"},{"id":"132055","messageId":"alpine.LFD.2.00.1001181433240.13231@localhost.localdomain","threadId":"22275","inReplyTo":"871vhnhydg.fsf@meyering.net","subject":"Re: [PATCH] grep: don't segfault","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2010-01-18T22:40:07Z","receivedAt":"2010-01-18T22:40:07Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 18 Jan 2010, Jim Meyering wrote:\n> \n> \tsz = xsize_t(st.st_size);\n> \ti = open(filename, O_RDONLY);\n> \tif (i < 0)\n> \t\tgoto err_ret;\n> \tdata = xmalloc(sz + 1);\n> \tif (st.st_size != read_in_full(i, data, sz)) {\n> \t\terror(\"'%s': short read %s\", filename, strerror(errno));\n> \t\tclose(i);\n> \t\tfree(data);\n> \t\treturn 0;\n> \t}\n> \tclose(i);\n> >>>     data[sz] = 0;    <======  added line\n\nHeh. Fredrik's threaded grep patch also fixed this, although he did the \n\"data[*sz] = 0;\" up front.\n\nSo an obvious ack on the whole thing (the xmalloc() itself makes it \nobvious that that thing should be NULL-terminated).\n\nThat said, I also suspect that we have a _lot_ of these patterns, and I \nwonder if we should just add a \n\n\tvoid *read_file(int fd, struct stat *st);\n\nhelper function, which reads a file and NULL-terminates it. Doing a\n\n\tgit grep -5 read_in_full\n\non the git sources, and it looks like there are several cases that \nbasically do that xmalloc+read_in_full+error handling pattern.\n\n\t\tLinus\n"}]}