minor problems in git.c

minor problems in git.c

[Robert Watson]

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

Re: minor problems in git.c

[Alex Riesen]

[-- Attachment #1: Type: text/plain, Size: 386 bytes --]

On 12/1/05, Robert Watson <robert.oo.watson@gmail.com> 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

[-- Attachment #2: 0001-used-stderr-for-error-output-and-build-git_command-more-careful.txt --]
[-- Type: text/plain, Size: 984 bytes --]

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



[-- Attachment #3: 0002-ENOENT-is-good-enough-no-access-check-needed.txt --]
[-- Type: text/plain, Size: 855 bytes --]

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

Re: minor problems in git.c

[Sven Verdoolaege]

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

Re: minor problems in git.c

[Alex Riesen]

On 12/1/05, Sven Verdoolaege <skimo@kotnet.org> 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.

Re: minor problems in git.c

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> 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 <raa.lkml@gmail.com>
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 <junkio@cox.net>

---

 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

Re: minor problems in git.c

[Alex Riesen]

On 12/2/05, Junio C Hamano <junkio@cox.net> 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!