# minor problems in git.c 6 messages from 2005-12-01 to 2005-12-02. Participants: Robert Watson, Alex Riesen, Sven Verdoolaege, Junio C Hamano. Thread: https://gitlist.dev/t/2721 ## Robert Watson, 2005-12-01 12:00 Subject: minor problems in git.c Message-ID: <72499e3b0512010400i1de76ed2la22cd745f811007f@mail.gmail.com> URL: https://gitlist.dev/e/72499e3b0512010400i1de76ed2la22cd745f811007f%40mail.gmail.com ``` Hi, There are some minor problems in git.c: (1) potential buffer overrun. strncat(&git_command[len], "/git-", sizeof(git_command) - len); len += 5; strncat(&git_command[len], argv[i], sizeof(git_command) - len); The first line will write one byte ('\0') beyond the end of git_command, when sizeof(git_command) - len == 5. The second line increase len by 5, without regarding how many bytes are written in the first line. It is possible to make len greater than sizeof(git_command), therefore make the third argument of the third line underflow, allowing almost any number of bytes from argv[1] to be copied. (2) environ int main(int argc, char **argv, char **envp) { ... execve(git_command, &argv[i], envp); ... } I am wondering whether the global variable "environ" could change when you do setenv. Would it be clear by using the "environ" as the third argument of evecve()? (3) printf("Failed to run command '%s': %s\n", git_command, strerror(errno)); should go to stderr? Regards, Robertoo ``` ## Alex Riesen, 2005-12-01 12:48 Subject: Re: minor problems in git.c Message-ID: <81b0412b0512010448u7fcdddacnd7de5df217ab3ca@mail.gmail.com> URL: https://gitlist.dev/e/81b0412b0512010448u7fcdddacnd7de5df217ab3ca%40mail.gmail.com In-Reply-To: <72499e3b0512010400i1de76ed2la22cd745f811007f@mail.gmail.com> ``` On 12/1/05, Robert Watson wrote: > There are some minor problems in git.c: I had the following patches in my tree for some time. Even forgot about them, sorry. The second on top of the first. - Use stderr for error output - Build git_command more careful - ENOENT is good enough for check of failed exec to show usage, no access() check needed Use stderr for error output and build git_command more careful --- git.c | 7 +++---- 1 files changed, 3 insertions(+), 4 deletions(-) 081fc78a8c8e640420ac7e44d93a2a45246f5c2f diff --git a/git.c b/git.c index bdd3f8d..9468b58 100644 --- a/git.c +++ b/git.c @@ -283,16 +283,15 @@ int main(int argc, char **argv, char **e len = strlen(git_command); prepend_to_path(git_command, len); - strncat(&git_command[len], "/git-", sizeof(git_command) - len); - len += 5; - strncat(&git_command[len], argv[i], sizeof(git_command) - len); + snprintf(git_command + len, sizeof(git_command) - len, "/git-%s", + argv[i]); if (access(git_command, X_OK)) usage(exec_path, "'%s' is not a git-command", argv[i]); /* execve() can only ever return if it fails */ execve(git_command, &argv[i], envp); - printf("Failed to run command '%s': %s\n", git_command, strerror(errno)); + fprintf(stderr, "git: '%s': %s\n", git_command, strerror(errno)); return 1; } -- 0.99.9.GIT ENOENT is good enough, no access() check needed --- git.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-) ac97adc8152a1e5ac78a03f218a3dab012bf8ba9 diff --git a/git.c b/git.c index 9468b58..c8c2b4a 100644 --- a/git.c +++ b/git.c @@ -286,12 +286,12 @@ int main(int argc, char **argv, char **e snprintf(git_command + len, sizeof(git_command) - len, "/git-%s", argv[i]); - if (access(git_command, X_OK)) - usage(exec_path, "'%s' is not a git-command", argv[i]); - /* execve() can only ever return if it fails */ execve(git_command, &argv[i], envp); - fprintf(stderr, "git: '%s': %s\n", git_command, strerror(errno)); + if ( ENOENT == errno ) + usage(exec_path, "'%s' is not a git-command", argv[i]); + else + fprintf(stderr, "git: '%s': %s\n", git_command, strerror(errno)); return 1; } -- 0.99.9.GIT ``` ## Sven Verdoolaege, 2005-12-01 13:51 Subject: Re: minor problems in git.c Message-ID: <20051201135113.GW8383MdfPADPa@greensroom.kotnet.org> URL: https://gitlist.dev/e/20051201135113.GW8383MdfPADPa%40greensroom.kotnet.org In-Reply-To: <81b0412b0512010448u7fcdddacnd7de5df217ab3ca@mail.gmail.com> ``` On Thu, Dec 01, 2005 at 01:48:35PM +0100, Alex Riesen wrote: > @@ -283,16 +283,15 @@ int main(int argc, char **argv, char **e > len = strlen(git_command); > prepend_to_path(git_command, len); > > - strncat(&git_command[len], "/git-", sizeof(git_command) - len); > - len += 5; > - strncat(&git_command[len], argv[i], sizeof(git_command) - len); > + snprintf(git_command + len, sizeof(git_command) - len, "/git-%s", > + argv[i]); Shouldn't you check the return value of snprintf > if (access(git_command, X_OK)) > usage(exec_path, "'%s' is not a git-command", argv[i]); or use the (possibly) truncated version of the command in the error message ? skimo ``` ## Alex Riesen, 2005-12-01 14:02 Subject: Re: minor problems in git.c Message-ID: <81b0412b0512010602l63ecev1ba03fb90d06e071@mail.gmail.com> URL: https://gitlist.dev/e/81b0412b0512010602l63ecev1ba03fb90d06e071%40mail.gmail.com In-Reply-To: <20051201135113.GW8383MdfPADPa@greensroom.kotnet.org> ``` On 12/1/05, Sven Verdoolaege wrote: > On Thu, Dec 01, 2005 at 01:48:35PM +0100, Alex Riesen wrote: > > @@ -283,16 +283,15 @@ int main(int argc, char **argv, char **e > > len = strlen(git_command); > > prepend_to_path(git_command, len); > > > > - strncat(&git_command[len], "/git-", sizeof(git_command) - len); > > - len += 5; > > - strncat(&git_command[len], argv[i], sizeof(git_command) - len); > > + snprintf(git_command + len, sizeof(git_command) - len, "/git-%s", > > + argv[i]); > > Shouldn't you check the return value of snprintf Probably. For the case where length of a git-command-name + --exec-prefix together are longer than PATH_MAX. > > if (access(git_command, X_OK)) > > usage(exec_path, "'%s' is not a git-command", argv[i]); > > or use the (possibly) truncated version of the command in the error message ? argv[i] is the command name, already as truncated as it can possibly be: ls-files, ls-tree, etc. Besides, the second path removes this access check altogether: - if (access(git_command, X_OK)) - usage(exec_path, "'%s' is not a git-command", argv[i]); - /* execve() can only ever return if it fails */ execve(git_command, &argv[i], envp); - fprintf(stderr, "git: '%s': %s\n", git_command, strerror(errno)); + if ( ENOENT == errno ) + usage(exec_path, "'%s' is not a git-command", argv[i]); + else + fprintf(stderr, "git: '%s': %s\n", git_command, strerror(errno)); It still has the call to usage, though. ``` ## Junio C Hamano, 2005-12-02 01:07 Subject: Re: minor problems in git.c Message-ID: <7v1x0wb936.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7v1x0wb936.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <81b0412b0512010602l63ecev1ba03fb90d06e071@mail.gmail.com> ``` Alex Riesen writes: >> Shouldn't you check the return value of snprintf > > Probably. For the case where length of a git-command-name + > --exec-prefix together are longer than PATH_MAX. Combined, something like this. -- >8 -- Subject: git wrapper: more careful argument stuffing From: Alex Riesen Date: Thu, 1 Dec 2005 13:48:35 +0100 - Use stderr for error output - Build git_command more careful - ENOENT is good enough for check of failed exec to show usage, no access() check needed [jc: Originally from Alex Riesen with inputs from Sven Verdoolaege mixed in.] Signed-off-by: Junio C Hamano --- git.c | 19 ++++++++++++------- 1 files changed, 12 insertions(+), 7 deletions(-) 6e3f1bf88fdce10ba5c0274e017667d21bb68359 diff --git a/git.c b/git.c index 0b10b6e..878c359 100644 --- a/git.c +++ b/git.c @@ -283,16 +283,21 @@ int main(int argc, char **argv, char **e len = strlen(git_command); prepend_to_path(git_command, len); - strncat(&git_command[len], "/git-", sizeof(git_command) - len); - len += 5; - strncat(&git_command[len], argv[i], sizeof(git_command) - len); - - if (access(git_command, X_OK)) - usage(exec_path, "'%s' is not a git-command", argv[i]); + len += snprintf(git_command + len, sizeof(git_command) - len, + "/git-%s", argv[i]); + if (sizeof(git_command) <= len) { + fprintf(stderr, "git: command name given is too long (%d)\n", len); + exit(1); + } /* execve() can only ever return if it fails */ execve(git_command, &argv[i], envp); - printf("Failed to run command '%s': %s\n", git_command, strerror(errno)); + + if (errno == ENOENT) + usage(exec_path, "'%s' is not a git-command", argv[i]); + + fprintf(stderr, "Failed to run command '%s': %s\n", + git_command, strerror(errno)); return 1; } -- 0.99.9.GIT ``` ## Alex Riesen, 2005-12-02 08:12 Subject: Re: minor problems in git.c Message-ID: <81b0412b0512020012m3bfbfe9fka1f412d70b2255d0@mail.gmail.com> URL: https://gitlist.dev/e/81b0412b0512020012m3bfbfe9fka1f412d70b2255d0%40mail.gmail.com In-Reply-To: <7v1x0wb936.fsf@assigned-by-dhcp.cox.net> ``` On 12/2/05, Junio C Hamano wrote: > >> Shouldn't you check the return value of snprintf > > > > Probably. For the case where length of a git-command-name + > > --exec-prefix together are longer than PATH_MAX. > > Combined, something like this. Thanks! ```