threads / discuss / 11969

read_branches_file ()

Subject: read_branches_file ()

## tl;dr

7 messages between Feb 8, 2008 and Feb 11, 2008.

replies: 6people: 4as markdown or json

H.Merijn Brand· Feb 8, 2008, 15:50 UTC · lore

I'm (again) trying to port git-1.5.4 to HP-UX, and I've already got rather far, but I'm hitting some stuff I cannot explain.

t5405-send-pack-rewind.sh fails the 'git fetch .. master:master' part in the setup, as deep down, read_branches_file () is called with in remote the name "..".

The file that it tries to open using git_path () is ".git/branches/.." That is weird. That is not a file, but a dir. "../.git/branches" would be more logical, but whatever. HP-UX 11.00 will gladly return a valid FILE * for opening a directory with fopen (), which, when read, will return anything but what is expected. So, maybe read_branches_file () should be protected against opening anything but files. Maybe with some stat () and S_ISREG ()'s.

Or has something gone wrong earlier on?
In my case, the returned url is 'l', which cannot be opened:

fatal: 'l': unable to chdir or not a git archive fatal: The remote end hung up unexpectedly

which is cast from upload-pack.c:main ()
-- 
H.Merijn Brand         Amsterdam Perl Mongers (http://amsterdam.pm.org/)
using & porting perl 5.6.2, 5.8.x, 5.10.x  on HP-UX 10.20, 11.00, 11.11,
& 11.23, SuSE 10.1 & 10.2, AIX 5.2, and Cygwin.       http://qa.perl.org
http://mirrors.develooper.com/hpux/            http://www.test-smoke.org
                        http://www.goldmark.org/jeff/stupid-disclaimers/
H.Merijn Brand· Feb 8, 2008, 16:03 UTC · re: H.Merijn Brand · lore

Re: read_branches_file ()

On Fri, 8 Feb 2008 16:50:08 +0100, "H.Merijn Brand" <h.m.brand@xs4all.nl> wrote:

Show 14 quoted lines
> I'm (again) trying to port git-1.5.4 to HP-UX, and I've already got rather
> far, but I'm hitting some stuff I cannot explain.
> 
> t5405-send-pack-rewind.sh fails the 'git fetch .. master:master' part in
> the setup, as deep down, read_branches_file () is called with in remote
> the name "..".
> 
> The file that it tries to open using git_path () is ".git/branches/.."
> That is weird. That is not a file, but a dir. "../.git/branches" would
> be more logical, but whatever. HP-UX 11.00 will gladly return a valid
> FILE * for opening a directory with fopen (), which, when read, will
> return anything but what is expected. So, maybe read_branches_file ()
> should be protected against opening anything but files. Maybe with some
> stat () and S_ISREG ()'s.

Something like this seems so `fix' this specific problem. Feel free to take another approach, t5405 now passes

--8<--- remote.c.diff --- remote.c.org 2008-01-27 09:04:18 +0100 +++ remote.c 2008-02-08 17:01:09 +0100

@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "remote.h"
 #include "refs.h"
+#include <sys/stat.h>

 static struct remote **remotes;
 static int allocated_remotes;
@@ -173,11 +174,15 @@ static void read_branches_file(struct re
        char *frag;
        char *branch;
        int n = slash ? slash - remote->name : 1000;
-       FILE *f = fopen(git_path("branches/%.*s", n, remote->name), "r");
+       char *gp = git_path ("branches/%.*s", n, remote->name);
+       struct stat st_buf;
+       FILE *f;
        char *s, *p;
        int len;

-       if (!f)
+       if (stat (gp, &st_buf) || S_ISDIR (st_buf.st_mode))
+               return;
+       if (!(f = fopen(gp, "r")))
                return;
        s = fgets(buffer, BUF_SIZE, f);
        fclose(f);
-->8---

> Or has something gone wrong earlier on?
> 
> In my case, the returned url is 'l', which cannot be opened:
> 
> fatal: 'l': unable to chdir or not a git archive
> fatal: The remote end hung up unexpectedly
> 
> which is cast from upload-pack.c:main ()

-- 
H.Merijn Brand         Amsterdam Perl Mongers (http://amsterdam.pm.org/)
using & porting perl 5.6.2, 5.8.x, 5.10.x  on HP-UX 10.20, 11.00, 11.11,
& 11.23, SuSE 10.1 & 10.2, AIX 5.2, and Cygwin.       http://qa.perl.org
http://mirrors.develooper.com/hpux/            http://www.test-smoke.org
                        http://www.goldmark.org/jeff/stupid-disclaimers/
Johannes Schindelin· Feb 8, 2008, 16:22 UTC · re: H.Merijn Brand · lore

Re: read_branches_file ()

Hi,
On Fri, 8 Feb 2008, H.Merijn Brand wrote:
Show 8 quoted lines
> --8<--- remote.c.diff
> --- remote.c.org        2008-01-27 09:04:18 +0100
> +++ remote.c    2008-02-08 17:01:09 +0100
> @@ -1,6 +1,7 @@
>  #include "cache.h"
>  #include "remote.h"
>  #include "refs.h"
> +#include <sys/stat.h>
This should not be necessary; we include all system headers in cache.h.
Show 6 quoted lines
> @@ -173,11 +174,15 @@ static void read_branches_file(struct re
>         char *frag;
>         char *branch;
>         int n = slash ? slash - remote->name : 1000;
> -       FILE *f = fopen(git_path("branches/%.*s", n, remote->name), "r");
> +       char *gp = git_path ("branches/%.*s", n, remote->name);

Please use a more descriptive variable name, such as "branches_file" or "branches_path".

Also, we only leave a space after operators like "for", "while", but not after function names.

> +       if (stat (gp, &st_buf) || S_ISDIR (st_buf.st_mode))
Again, please remove the spaces after "stat" and "S_ISDIR".

Other than that, the patch looks obviously correct: please resubmit with a nice commit message and a sign-off.

Thanks, Dscho

H.Merijn Brand· Feb 8, 2008, 16:49 UTC · re: Johannes Schindelin · lore

Re: read_branches_file ()

On Fri, 8 Feb 2008 16:22:45 +0000 (GMT), Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 14 quoted lines
> Hi,
> 
> On Fri, 8 Feb 2008, H.Merijn Brand wrote:
> 
> > --8<--- remote.c.diff
> > --- remote.c.org        2008-01-27 09:04:18 +0100
> > +++ remote.c    2008-02-08 17:01:09 +0100
> > @@ -1,6 +1,7 @@
> >  #include "cache.h"
> >  #include "remote.h"
> >  #include "refs.h"
> > +#include <sys/stat.h>
> 
> This should not be necessary; we include all system headers in cache.h.
dropped
Show 9 quoted lines
> > @@ -173,11 +174,15 @@ static void read_branches_file(struct re
> >         char *frag;
> >         char *branch;
> >         int n = slash ? slash - remote->name : 1000;
> > -       FILE *f = fopen(git_path("branches/%.*s", n, remote->name), "r");
> > +       char *gp = git_path ("branches/%.*s", n, remote->name);
> 
> Please use a more descriptive variable name, such as "branches_file" or 
> "branches_path".
took another approach, as that also addresses the other fopen () call
> Also, we only leave a space after operators like "for", "while", but not 
> after function names.

This patch was not sent to be applied as-is, only as a proof-of-concept Not that I agree to the layout/indentation, the new patch is sent trying to follow what you use.

> > +       if (stat (gp, &st_buf) || S_ISDIR (st_buf.st_mode))
> 
> Again, please remove the spaces after "stat" and "S_ISDIR".
you said please :)
> Other than that, the patch looks obviously correct: please resubmit with a 
> nice commit message and a sign-off.
Done
-- 
H.Merijn Brand         Amsterdam Perl Mongers (http://amsterdam.pm.org/)
using & porting perl 5.6.2, 5.8.x, 5.10.x  on HP-UX 10.20, 11.00, 11.11,
& 11.23, SuSE 10.1 & 10.2, AIX 5.2, and Cygwin.       http://qa.perl.org
http://mirrors.develooper.com/hpux/            http://www.test-smoke.org
                        http://www.goldmark.org/jeff/stupid-disclaimers/
Andreas Ericsson· Feb 11, 2008, 10:00 UTC · re: H.Merijn Brand · lore

Re: read_branches_file ()

H.Merijn Brand wrote:
> -       if (!f)
> +       if (stat (gp, &st_buf) || S_ISDIR (st_buf.st_mode))
Shouldn't this be
	if (stat(gp, &st_buf) || !S_ISREG(st_buf.st_mode))
?

Otherwise, you might end up opening a FIFO, a socket or a block/char special, which is obviously undesired.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231
H.Merijn Brand· Feb 11, 2008, 10:17 UTC · re: Andreas Ericsson · lore

Re: read_branches_file ()

On Mon, 11 Feb 2008 11:00:52 +0100, Andreas Ericsson <ae@op5.se> wrote:
Show 6 quoted lines
> H.Merijn Brand wrote:
> > -       if (!f)
> > +       if (stat (gp, &st_buf) || S_ISDIR (st_buf.st_mode))
> 
> Shouldn't this be
> 	if (stat(gp, &st_buf) || !S_ISREG(st_buf.st_mode))
Yes, see several follow-ups in this thread.
> ?
> 
> Otherwise, you might end up opening a FIFO, a socket or a
> block/char special, which is obviously undesired.

At the moment of the patch I was unsure if these were supported :) At least FIFO's and character devices are less error-prone than DIR's

-- 
H.Merijn Brand         Amsterdam Perl Mongers (http://amsterdam.pm.org/)
using & porting perl 5.6.2, 5.8.x, 5.10.x  on HP-UX 10.20, 11.00, 11.11,
& 11.23, SuSE 10.1 & 10.2, AIX 5.2, and Cygwin.       http://qa.perl.org
http://mirrors.develooper.com/hpux/            http://www.test-smoke.org
                        http://www.goldmark.org/jeff/stupid-disclaimers/
Daniel Barkalow· Feb 8, 2008, 17:18 UTC · re: H.Merijn Brand · lore

Re: read_branches_file ()

On Fri, 8 Feb 2008, H.Merijn Brand wrote:
Show 16 quoted lines
> I'm (again) trying to port git-1.5.4 to HP-UX, and I've already got rather
> far, but I'm hitting some stuff I cannot explain.
> 
> t5405-send-pack-rewind.sh fails the 'git fetch .. master:master' part in
> the setup, as deep down, read_branches_file () is called with in remote
> the name "..".
> 
> The file that it tries to open using git_path () is ".git/branches/.."
> That is weird. That is not a file, but a dir. "../.git/branches" would
> be more logical, but whatever. HP-UX 11.00 will gladly return a valid
> FILE * for opening a directory with fopen (), which, when read, will
> return anything but what is expected. So, maybe read_branches_file ()
> should be protected against opening anything but files. Maybe with some
> stat () and S_ISREG ()'s.
> 
> Or has something gone wrong earlier on?

It's doing this because it thinks you might have a remote configured with the name "..", which should probably be blocked at a higher level, if only for general sanity.

	-Daniel
*This .sig left intentionally blank*

← back to recent threads