{"thread":{"id":"2721","subject":"minor problems in git.c","startedAt":"2005-12-01T12:00:54Z","lastAt":"2005-12-02T08:12:05Z","messageCount":6,"participants":["Robert Watson","Alex Riesen","Sven Verdoolaege","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"13040","messageId":"72499e3b0512010400i1de76ed2la22cd745f811007f@mail.gmail.com","threadId":"2721","inReplyTo":null,"subject":"minor problems in git.c","fromName":"Robert Watson","fromEmail":"robert.oo.watson@gmail.com","sentAt":"2005-12-01T12:00:54Z","receivedAt":"2005-12-01T12:00:54Z","isPatch":false,"sender":{"key":"robert.oo.watson@gmail.com","avatar":null},"body":"Hi,\n\nThere are some minor problems in git.c:\n\n(1) potential buffer overrun.\n\n strncat(&git_command[len], \"/git-\", sizeof(git_command) - len);\n len += 5;\n strncat(&git_command[len], argv[i], sizeof(git_command) - len);\n\nThe first line will write one byte ('\\0') beyond the end of\ngit_command, when sizeof(git_command) - len == 5.\n\nThe second line increase len by 5, without regarding how many bytes\nare written in the first line. It is possible to make len greater\nthan sizeof(git_command), therefore make the third argument of the\nthird line underflow, allowing almost any number of bytes from argv[1]\nto be copied.\n\n(2) environ\n\nint main(int argc, char **argv, char **envp)\n{\n ...\n execve(git_command, &argv[i], envp);\n ...\n}\n\nI am wondering whether the global variable \"environ\" could change when\nyou do setenv. Would it be clear by using the \"environ\" as the third\nargument of evecve()?\n\n(3) printf(\"Failed to run command '%s': %s\\n\", git_command, strerror(errno));\nshould go to stderr?\n\nRegards,\nRobertoo\n"},{"id":"13041","messageId":"81b0412b0512010448u7fcdddacnd7de5df217ab3ca@mail.gmail.com","threadId":"2721","inReplyTo":"72499e3b0512010400i1de76ed2la22cd745f811007f@mail.gmail.com","subject":"Re: minor problems in git.c","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2005-12-01T12:48:35Z","receivedAt":"2005-12-01T12:48:35Z","isPatch":false,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 12/1/05, Robert Watson wrote:\n> There are some minor problems in git.c:\n\nI had the following patches in my tree for some time. Even forgot\nabout them, sorry.\nThe second on top of the first.\n\n- Use stderr for error output\n- Build git_command more careful\n- ENOENT is good enough for check of failed exec to show usage, no\naccess() check needed\n\n\nUse stderr for error output and build git_command more careful\n\n---\n\n git.c | 7 +++----\n 1 files changed, 3 insertions(+), 4 deletions(-)\n\n081fc78a8c8e640420ac7e44d93a2a45246f5c2f\ndiff --git a/git.c b/git.c\nindex bdd3f8d..9468b58 100644\n--- a/git.c\n+++ b/git.c\n@@ -283,16 +283,15 @@ int main(int argc, char **argv, char **e\n \tlen = strlen(git_command);\n \tprepend_to_path(git_command, len);\n \n-\tstrncat(&git_command[len], \"/git-\", sizeof(git_command) - len);\n-\tlen += 5;\n-\tstrncat(&git_command[len], argv[i], sizeof(git_command) - len);\n+\tsnprintf(git_command + len, sizeof(git_command) - len, \"/git-%s\",\n+\t\t argv[i]);\n \n \tif (access(git_command, X_OK))\n \t\tusage(exec_path, \"'%s' is not a git-command\", argv[i]);\n \n \t/* execve() can only ever return if it fails */\n \texecve(git_command, &argv[i], envp);\n-\tprintf(\"Failed to run command '%s': %s\\n\", git_command, strerror(errno));\n+\tfprintf(stderr, \"git: '%s': %s\\n\", git_command, strerror(errno));\n \n \treturn 1;\n }\n-- \n0.99.9.GIT\n\n\n\n\nENOENT is good enough, no access() check needed\n\n---\n\n git.c | 8 ++++----\n 1 files changed, 4 insertions(+), 4 deletions(-)\n\nac97adc8152a1e5ac78a03f218a3dab012bf8ba9\ndiff --git a/git.c b/git.c\nindex 9468b58..c8c2b4a 100644\n--- a/git.c\n+++ b/git.c\n@@ -286,12 +286,12 @@ int main(int argc, char **argv, char **e\n \tsnprintf(git_command + len, sizeof(git_command) - len, \"/git-%s\",\n \t\t argv[i]);\n \n-\tif (access(git_command, X_OK))\n-\t\tusage(exec_path, \"'%s' is not a git-command\", argv[i]);\n-\n \t/* execve() can only ever return if it fails */\n \texecve(git_command, &argv[i], envp);\n-\tfprintf(stderr, \"git: '%s': %s\\n\", git_command, strerror(errno));\n+ if ( ENOENT == errno )\n+\t\tusage(exec_path, \"'%s' is not a git-command\", argv[i]);\n+ else\n+\t\tfprintf(stderr, \"git: '%s': %s\\n\", git_command, strerror(errno));\n \n \treturn 1;\n }\n-- \n0.99.9.GIT\n\n"},{"id":"13047","messageId":"20051201135113.GW8383MdfPADPa@greensroom.kotnet.org","threadId":"2721","inReplyTo":"81b0412b0512010448u7fcdddacnd7de5df217ab3ca@mail.gmail.com","subject":"Re: minor problems in git.c","fromName":"Sven Verdoolaege","fromEmail":"skimo@kotnet.org","sentAt":"2005-12-01T13:51:13Z","receivedAt":"2005-12-01T13:51:13Z","isPatch":false,"sender":{"key":"skimo@kotnet.org","avatar":null},"body":"On Thu, Dec 01, 2005 at 01:48:35PM +0100, Alex Riesen wrote:\n> @@ -283,16 +283,15 @@ int main(int argc, char **argv, char **e\n> \tlen = strlen(git_command);\n> \tprepend_to_path(git_command, len);\n> \n> -\tstrncat(&git_command[len], \"/git-\", sizeof(git_command) - len);\n> -\tlen += 5;\n> -\tstrncat(&git_command[len], argv[i], sizeof(git_command) - len);\n> +\tsnprintf(git_command + len, sizeof(git_command) - len, \"/git-%s\",\n> +\t\t argv[i]);\n\nShouldn't you check the return value of snprintf\n\n> \tif (access(git_command, X_OK))\n> \t\tusage(exec_path, \"'%s' is not a git-command\", argv[i]);\n\nor use the (possibly) truncated version of the command in the error message ?\n\nskimo\n"},{"id":"13048","messageId":"81b0412b0512010602l63ecev1ba03fb90d06e071@mail.gmail.com","threadId":"2721","inReplyTo":"20051201135113.GW8383MdfPADPa@greensroom.kotnet.org","subject":"Re: minor problems in git.c","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2005-12-01T14:02:16Z","receivedAt":"2005-12-01T14:02:16Z","isPatch":false,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 12/1/05, Sven Verdoolaege wrote:\n> On Thu, Dec 01, 2005 at 01:48:35PM +0100, Alex Riesen wrote:\n> > @@ -283,16 +283,15 @@ int main(int argc, char **argv, char **e\n> > len = strlen(git_command);\n> > prepend_to_path(git_command, len);\n> >\n> > - strncat(&git_command[len], \"/git-\", sizeof(git_command) - len);\n> > - len += 5;\n> > - strncat(&git_command[len], argv[i], sizeof(git_command) - len);\n> > + snprintf(git_command + len, sizeof(git_command) - len, \"/git-%s\",\n> > + argv[i]);\n>\n> Shouldn't you check the return value of snprintf\n\nProbably. For the case where length of a git-command-name +\n--exec-prefix together are longer than PATH_MAX.\n\n> > if (access(git_command, X_OK))\n> > usage(exec_path, \"'%s' is not a git-command\", argv[i]);\n>\n> or use the (possibly) truncated version of the command in the error message ?\n\nargv[i] is the command name, already as truncated as it can possibly\nbe: ls-files, ls-tree, etc. Besides, the second path removes this\naccess check altogether:\n\n-\tif (access(git_command, X_OK))\n-\t\tusage(exec_path, \"'%s' is not a git-command\", argv[i]);\n-\n \t/* execve() can only ever return if it fails */\n \texecve(git_command, &argv[i], envp);\n-\tfprintf(stderr, \"git: '%s': %s\\n\", git_command, strerror(errno));\n+ if ( ENOENT == errno )\n+\t\tusage(exec_path, \"'%s' is not a git-command\", argv[i]);\n+ else\n+\t\tfprintf(stderr, \"git: '%s': %s\\n\", git_command, strerror(errno));\n\n\nIt still has the call to usage, though.\n"},{"id":"13083","messageId":"7v1x0wb936.fsf@assigned-by-dhcp.cox.net","threadId":"2721","inReplyTo":"81b0412b0512010602l63ecev1ba03fb90d06e071@mail.gmail.com","subject":"Re: minor problems in git.c","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-12-02T01:07:09Z","receivedAt":"2005-12-02T01:07:09Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Riesen writes:\n\n>> Shouldn't you check the return value of snprintf\n>\n> Probably. For the case where length of a git-command-name +\n> --exec-prefix together are longer than PATH_MAX.\n\nCombined, something like this.\n\n-- >8 --\nSubject: git wrapper: more careful argument stuffing\nFrom: Alex Riesen \nDate: Thu, 1 Dec 2005 13:48:35 +0100\n\n - Use stderr for error output\n - Build git_command more careful\n - ENOENT is good enough for check of failed exec to show usage, no\n access() check needed\n\n[jc: Originally from Alex Riesen with inputs from Sven\n Verdoolaege mixed in.]\n\nSigned-off-by: Junio C Hamano \n\n---\n\n git.c | 19 ++++++++++++-------\n 1 files changed, 12 insertions(+), 7 deletions(-)\n\n6e3f1bf88fdce10ba5c0274e017667d21bb68359\ndiff --git a/git.c b/git.c\nindex 0b10b6e..878c359 100644\n--- a/git.c\n+++ b/git.c\n@@ -283,16 +283,21 @@ int main(int argc, char **argv, char **e\n \tlen = strlen(git_command);\n \tprepend_to_path(git_command, len);\n \n-\tstrncat(&git_command[len], \"/git-\", sizeof(git_command) - len);\n-\tlen += 5;\n-\tstrncat(&git_command[len], argv[i], sizeof(git_command) - len);\n-\n-\tif (access(git_command, X_OK))\n-\t\tusage(exec_path, \"'%s' is not a git-command\", argv[i]);\n+\tlen += snprintf(git_command + len, sizeof(git_command) - len,\n+\t\t\t\"/git-%s\", argv[i]);\n+\tif (sizeof(git_command) <= len) {\n+\t\tfprintf(stderr, \"git: command name given is too long (%d)\\n\", len);\n+\t\texit(1);\n+\t}\n \n \t/* execve() can only ever return if it fails */\n \texecve(git_command, &argv[i], envp);\n-\tprintf(\"Failed to run command '%s': %s\\n\", git_command, strerror(errno));\n+\n+\tif (errno == ENOENT)\n+\t\tusage(exec_path, \"'%s' is not a git-command\", argv[i]);\n+\n+\tfprintf(stderr, \"Failed to run command '%s': %s\\n\",\n+\t\tgit_command, strerror(errno));\n \n \treturn 1;\n }\n-- \n0.99.9.GIT\n"},{"id":"13092","messageId":"81b0412b0512020012m3bfbfe9fka1f412d70b2255d0@mail.gmail.com","threadId":"2721","inReplyTo":"7v1x0wb936.fsf@assigned-by-dhcp.cox.net","subject":"Re: minor problems in git.c","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2005-12-02T08:12:05Z","receivedAt":"2005-12-02T08:12:05Z","isPatch":false,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 12/2/05, Junio C Hamano wrote:\n> >> Shouldn't you check the return value of snprintf\n> >\n> > Probably. For the case where length of a git-command-name +\n> > --exec-prefix together are longer than PATH_MAX.\n>\n> Combined, something like this.\n\nThanks!\n"}]}