[PATCH] opening files in remote.c should ensure it is opening a file

[PATCH] opening files in remote.c should ensure it is opening a file

[H.Merijn Brand]

HP-UX allows directories to be opened with fopen (path, "r"), which
will cause some translations that expect to read files, read dirs
instead. This patch makes sure the two fopen () calls in remote.c
only open the file if it is a file.

Signed-off-by: H.Merijn Brand <h.m.brand@xs4all.nl>
---

diff -pur git-1.5.4a/remote.c git-1.5.4b/remote.c
--- git-1.5.4a/remote.c    2008-01-27 09:04:18 +0100
+++ git-1.5.4/remote.c     2008-02-08 17:38:43 +0100
@@ -121,9 +121,18 @@ static struct branch *make_branch(const
        return branches[empty];
 }

+/* Helper function to ensure that we are opening a file and not a directory */
+static FILE *open_file(char *full_path)
+{
+       struct stat st_buf;
+       if (stat(full_path, &st_buf) || !S_ISREG(st_buf.st_mode))
+               return NULL;
+       return (fopen(full_path, "r"));
+}
+
 static void read_remotes_file(struct remote *remote)
 {
-       FILE *f = fopen(git_path("remotes/%s", remote->name), "r");
+       FILE *f = open_file(git_path("remotes/%s", remote->name));

        if (!f)
                return;
@@ -173,7 +182,7 @@ 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");
+       FILE *f = open_file(git_path("branches/%.*s", n, remote->name));
        char *s, *p;
        int len;

--
git-1.5.4

-- 
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/

Re: [PATCH] opening files in remote.c should ensure it is opening a file

[Mike Ralphson]

On Feb 8, 2008 4:46 PM, H.Merijn Brand <h.m.brand@xs4all.nl> wrote:
> HP-UX allows directories to be opened with fopen (path, "r"), which
> will cause some translations that expect to read files, read dirs
> instead. This patch makes sure the two fopen () calls in remote.c
> only open the file if it is a file.
>
> Signed-off-by: H.Merijn Brand <h.m.brand@xs4all.nl>

Many thanks, this is also required for AIX. I had got some way to
tracking it down, but I thought it was an issue with strbuf. So...

Tested-by: Mike Ralphson <mike.ralphson@gmail.com>

Your other fix there [- if (!strbuf_avail(sb)) / + if
(strbuf_avail(sb) < 64) ] is, guess what, also required on AIX.

Thanks again.

Re: [PATCH] opening files in remote.c should ensure it is opening a file

[H.Merijn Brand]

On Fri, 8 Feb 2008 17:25:52 +0000, "Mike Ralphson" <mike.ralphson@gmail.com>
wrote:

> On Feb 8, 2008 4:46 PM, H.Merijn Brand <h.m.brand@xs4all.nl> wrote:
> > HP-UX allows directories to be opened with fopen (path, "r"), which
> > will cause some translations that expect to read files, read dirs
> > instead. This patch makes sure the two fopen () calls in remote.c
> > only open the file if it is a file.
> >
> > Signed-off-by: H.Merijn Brand <h.m.brand@xs4all.nl>
> 
> Many thanks, this is also required for AIX. I had got some way to
> tracking it down, but I thought it was an issue with strbuf. So...
> 
> Tested-by: Mike Ralphson <mike.ralphson@gmail.com>
> 
> Your other fix there [- if (!strbuf_avail(sb)) / + if
> (strbuf_avail(sb) < 64) ] is, guess what, also required on AIX.
> 
> Thanks again.

Not there yet ...

$ cat do-tests
#!/bin/sh

export TAR=ntar
rm -f *.err
for t in t[0-9]*.sh ; do
    echo $t
    sh $t > test.err 2>&1 || mv test.err $t.err
    rm -f test.err
    done
$

197509 -rw-rw-rw- 1 merijn softwr 1633 Feb  8 18:03 t5302-pack-index.sh.err
196846 -rw-rw-rw- 1 merijn softwr  943 Feb  8 18:04 t5500-fetch-pack.sh.err
203431 -rw-rw-rw- 1 merijn softwr  344 Feb  8 18:05 t5600-clone-fail-cleanup.sh.err
202602 -rw-rw-rw- 1 merijn softwr  458 Feb  8 18:05 t5701-clone-local.sh.err
202761 -rw-rw-rw- 1 merijn softwr 3039 Feb  8 18:06 t6002-rev-list-bisect.sh.err
202641 -rw-rw-rw- 1 merijn softwr 3980 Feb  8 18:06 t6003-rev-list-topo-order.sh.err
202731 -rw-rw-rw- 1 merijn softwr  899 Feb  8 18:06 t6022-merge-rename.sh.err
197510 -rw-rw-rw- 1 merijn softwr 1340 Feb  8 18:08 t7201-co.sh.err
202705 -rw-rw-rw- 1 merijn softwr  149 Feb  8 18:09 t9300-fast-import.sh.err
197051 -rw-rw-rw- 1 merijn softwr 1651 Feb  8 18:09 t9301-fast-export.sh.err

http://www.xs4all.nl/~procura/git-1.5.3-1123ipf.tar

Tips welcome :)

-- 
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/

Re: [PATCH] opening files in remote.c should ensure it is opening a file

[H.Merijn Brand]

On Fri, 8 Feb 2008 21:04:47 +0100, "H.Merijn Brand" <h.m.brand@xs4all.nl>
wrote:

> $ cat do-tests
> #!/bin/sh
> 
> export TAR=ntar
> rm -f *.err
> for t in t[0-9]*.sh ; do
>     echo $t
>     sh $t > test.err 2>&1 || mv test.err $t.err
>     rm -f test.err
>     done
> $
> 
> 197509 -rw-rw-rw- 1 merijn softwr 1633 Feb  8 18:03 t5302-pack-index.sh.err
> 196846 -rw-rw-rw- 1 merijn softwr  943 Feb  8 18:04 t5500-fetch-pack.sh.err
> 203431 -rw-rw-rw- 1 merijn softwr  344 Feb  8 18:05 t5600-clone-fail-cleanup.sh.err
> 202602 -rw-rw-rw- 1 merijn softwr  458 Feb  8 18:05 t5701-clone-local.sh.err
> 202761 -rw-rw-rw- 1 merijn softwr 3039 Feb  8 18:06 t6002-rev-list-bisect.sh.err
> 202641 -rw-rw-rw- 1 merijn softwr 3980 Feb  8 18:06 t6003-rev-list-topo-order.sh.err
> 202731 -rw-rw-rw- 1 merijn softwr  899 Feb  8 18:06 t6022-merge-rename.sh.err
> 197510 -rw-rw-rw- 1 merijn softwr 1340 Feb  8 18:08 t7201-co.sh.err
> 202705 -rw-rw-rw- 1 merijn softwr  149 Feb  8 18:09 t9300-fast-import.sh.err
> 197051 -rw-rw-rw- 1 merijn softwr 1651 Feb  8 18:09 t9301-fast-export.sh.err
> 
> http://www.xs4all.nl/~procura/git-1.5.3-1123ipf.tar
> 
> Tips welcome :)

Most bizarre workaround found for clone (the first 4 failures):
--8<---
diff -pur /a5/pro/3gl/LINUX/git-1.5.4/git-clone.sh git-clone.sh
--- a/git-1.5.4/git-clone.sh  2008-02-02 05:09:01 +0100
+++ b/git-1.5.4/git-clone.sh  2008-02-18 10:03:26 +0100
@@ -368,7 +368,8 @@ yes)
                '') git-fetch-pack --all -k $quiet $depth $no_progress "$repo";;
                *) git-fetch-pack --all -k $quiet "$upload_pack" $depth $no_progress "$repo" ;;
                esac >"$GIT_DIR/CLONE_HEAD" ||
-                       die "fetch-pack from '$repo' failed."
+                       exit 1
+                       # die "fetch-pack from '$repo' failed."
                ;;
        esac
        ;;
-->8---

4 down, 6 to go
197225 -rw-rw-rw- 1 merijn softwr  7246 Feb 18 09:58 t6002-rev-list-bisect.sh.err
197111 -rw-rw-rw- 1 merijn softwr 10763 Feb 18 09:58 t6003-rev-list-topo-order.sh.err
197190 -rw-rw-rw- 1 merijn softwr 17903 Feb 18 09:58 t6022-merge-rename.sh.err
196841 -rw-rw-rw- 1 merijn softwr  9299 Feb 18 10:00 t7201-co.sh.err
196928 -rw-rw-rw- 1 merijn softwr   484 Feb 18 10:01 t9300-fast-import.sh.err
196683 -rw-rw-rw- 1 merijn softwr  5035 Feb 18 10:01 t9301-fast-export.sh.err

-- 
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/

Re: [PATCH] opening files in remote.c should ensure it is opening a file

[Junio C Hamano]

"H.Merijn Brand" <h.m.brand@xs4all.nl> writes:

> Most bizarre workaround found for clone (the first 4 failures):
> --8<---
> diff -pur /a5/pro/3gl/LINUX/git-1.5.4/git-clone.sh git-clone.sh
> --- a/git-1.5.4/git-clone.sh  2008-02-02 05:09:01 +0100
> +++ b/git-1.5.4/git-clone.sh  2008-02-18 10:03:26 +0100
> @@ -368,7 +368,8 @@ yes)
>                 '') git-fetch-pack --all -k $quiet $depth $no_progress "$repo";;
>                 *) git-fetch-pack --all -k $quiet "$upload_pack" $depth $no_progress "$repo" ;;
>                 esac >"$GIT_DIR/CLONE_HEAD" ||
> -                       die "fetch-pack from '$repo' failed."
> +                       exit 1
> +                       # die "fetch-pack from '$repo' failed."
>                 ;;
>         esac
>         ;;

That sounds *very* broken.

Is your /bin/sh really a variant of Bourne?

If HP-UX is broken in a similar way as Solaris is, in that it
installs a non-POSIX shell under /bin/sh and offers a Korn in
/bin/ksh, "make SHELL_PATH=/bin/ksh" may help.

Re: [PATCH] opening files in remote.c should ensure it is opening a file

[H.Merijn Brand]

On Mon, 18 Feb 2008 01:30:39 -0800, Junio C Hamano <gitster@pobox.com> wrote:

> "H.Merijn Brand" <h.m.brand@xs4all.nl> writes:
> 
> > Most bizarre workaround found for clone (the first 4 failures):
> > --8<---
> > diff -pur /a5/pro/3gl/LINUX/git-1.5.4/git-clone.sh git-clone.sh
> > --- a/git-1.5.4/git-clone.sh  2008-02-02 05:09:01 +0100
> > +++ b/git-1.5.4/git-clone.sh  2008-02-18 10:03:26 +0100
> > @@ -368,7 +368,8 @@ yes)
> >                 '') git-fetch-pack --all -k $quiet $depth $no_progress "$repo";;
> >                 *) git-fetch-pack --all -k $quiet "$upload_pack" $depth $no_progress "$repo" ;;
> >                 esac >"$GIT_DIR/CLONE_HEAD" ||
> > -                       die "fetch-pack from '$repo' failed."
> > +                       exit 1
> > +                       # die "fetch-pack from '$repo' failed."
> >                 ;;
> >         esac
> >         ;;
> 
> That sounds *very* broken.

Indeed, and trying to see if eval or exiting from with a sub caused
this weird behaviour, I failed to come up with a simple test script
to prove this.

> Is your /bin/sh really a variant of Bourne?

Yes

 NAME
      sh - overview of various system shells

 SYNOPSIS
    POSIX Shell:
      sh [+-aefhikmnoprstuvx] [+-o option] ...  [-c string] [arg ...]

      rsh [+-aefhikmnoprstuvx] [+-o option] ...  [-c string] [arg ...]

    Korn Shell:
      ksh [+-aefhikmnoprstuvx] [+-o option] ...  [-c string] [arg ...]

      rksh [+-aefhikmnoprstuvx] [+-o option] ...  [-c string] [arg ...]

    C Shell:
      csh [-cefinstvxTVX] [command_file] [argument_list ...]

    Key Shell:
      keysh

> If HP-UX is broken in a similar way as Solaris is, in that it
> installs a non-POSIX shell under /bin/sh and offers a Korn in
> /bin/ksh, "make SHELL_PATH=/bin/ksh" may help.

$ path -al sh ksh
  27231 100555 -r-x    2      bin    586136  27 Aug 2004 03:36 /usr/bin/sh
   1744 100555 -r-x    1      bin   1219780  27 Aug 2004 03:36 /sbin/sh
   3206 100555 -r-x    2      bin    446904  27 Aug 2004 03:20 /usr/bin/ksh

And running all with ksh only makes things worse!

$ cat t0000-basic.sh.err
t0000-basic.sh[31]: !:  not found
test_expect_success[31]: !:  not found
test_expect_success[31]: !:  not found
test_expect_failure[31]: !:  not found
test_expect_success[31]: !:  not found
test_expect_success[31]: !:  not found
test_expect_success[31]: !:  not found
test_expect_failure[31]: !:  not found
:
:



-- 
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/

Re: [PATCH] opening files in remote.c should ensure it is opening a file

[Daniel Barkalow]

On Fri, 8 Feb 2008, Mike Ralphson wrote:

> On Feb 8, 2008 4:46 PM, H.Merijn Brand <h.m.brand@xs4all.nl> wrote:
> > HP-UX allows directories to be opened with fopen (path, "r"), which
> > will cause some translations that expect to read files, read dirs
> > instead. This patch makes sure the two fopen () calls in remote.c
> > only open the file if it is a file.
> >
> > Signed-off-by: H.Merijn Brand <h.m.brand@xs4all.nl>
> 
> Many thanks, this is also required for AIX. I had got some way to
> tracking it down, but I thought it was an issue with strbuf. So...

Does the following help? We really ought to know that ".." must be a path 
literal (and there obviously should be more limitations on nicknames for 
remotes, but I haven't figured out what they should be yet).

	-Daniel
*This .sig left intentionally blank*

diff --git a/remote.c b/remote.c
index 0e00680..83a3d9d 100644
--- a/remote.c
+++ b/remote.c
@@ -348,7 +348,7 @@ struct remote *remote_get(const char *name)
        if (!name)
                name = default_remote_name;
        ret = make_remote(name, 0);
-       if (name[0] != '/') {
+       if (name[0] != '/' && strcmp(name, "..")) {
                if (!ret->url)
                        read_remotes_file(ret);
                if (!ret->url)

Re: [PATCH] opening files in remote.c should ensure it is opening a file

[Johannes Schindelin]

Hi,

On Fri, 8 Feb 2008, Daniel Barkalow wrote:

> On Fri, 8 Feb 2008, Mike Ralphson wrote:
> 
> > On Feb 8, 2008 4:46 PM, H.Merijn Brand <h.m.brand@xs4all.nl> wrote:
> > > HP-UX allows directories to be opened with fopen (path, "r"), which
> > > will cause some translations that expect to read files, read dirs
> > > instead. This patch makes sure the two fopen () calls in remote.c
> > > only open the file if it is a file.
> > >
> > > Signed-off-by: H.Merijn Brand <h.m.brand@xs4all.nl>
> > 
> > Many thanks, this is also required for AIX. I had got some way to
> > tracking it down, but I thought it was an issue with strbuf. So...
> 
> Does the following help? We really ought to know that ".." must be a path 
> literal (and there obviously should be more limitations on nicknames for 
> remotes, but I haven't figured out what they should be yet).
> 
> 	-Daniel
> *This .sig left intentionally blank*
> 
> diff --git a/remote.c b/remote.c
> index 0e00680..83a3d9d 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -348,7 +348,7 @@ struct remote *remote_get(const char *name)
>         if (!name)
>                 name = default_remote_name;
>         ret = make_remote(name, 0);
> -       if (name[0] != '/') {
> +       if (name[0] != '/' && strcmp(name, "..")) {
>                 if (!ret->url)
>                         read_remotes_file(ret);
>                 if (!ret->url)

You'll need to check for ".", too: "git pull . <branch>" was originally 
the only way to merge a local branch, and it is still valid.

Ciao,
Dscho

Re: [PATCH] opening files in remote.c should ensure it is opening a file

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> diff --git a/remote.c b/remote.c
> index 0e00680..83a3d9d 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -348,7 +348,7 @@ struct remote *remote_get(const char *name)
>         if (!name)
>                 name = default_remote_name;
>         ret = make_remote(name, 0);
> -       if (name[0] != '/') {
> +       if (name[0] != '/' && strcmp(name, "..")) {
>                 if (!ret->url)
>                         read_remotes_file(ret);
>                 if (!ret->url)

Perhaps "static int valid_remote_nick(const char*)" is needed?
I'd say we can limit it to something like:

static int valid_remote_nick(const char *name)
{
	if (!name[0] || /* not empty */
            (name[0] == '.' && /* not "." */
             (!name[1] || /* not ".." */
              (name[1] == '.' && !name[2]))))
		return 0;
	return !!strchr(name, '/'); /* no slash */
}

Re: [PATCH] opening files in remote.c should ensure it is opening a file

[Daniel Barkalow]

On Fri, 8 Feb 2008, Junio C Hamano wrote:

> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > diff --git a/remote.c b/remote.c
> > index 0e00680..83a3d9d 100644
> > --- a/remote.c
> > +++ b/remote.c
> > @@ -348,7 +348,7 @@ struct remote *remote_get(const char *name)
> >         if (!name)
> >                 name = default_remote_name;
> >         ret = make_remote(name, 0);
> > -       if (name[0] != '/') {
> > +       if (name[0] != '/' && strcmp(name, "..")) {
> >                 if (!ret->url)
> >                         read_remotes_file(ret);
> >                 if (!ret->url)
> 
> Perhaps "static int valid_remote_nick(const char*)" is needed?
> I'd say we can limit it to something like:
> 
> static int valid_remote_nick(const char *name)
> {
> 	if (!name[0] || /* not empty */
>             (name[0] == '.' && /* not "." */
>              (!name[1] || /* not ".." */
>               (name[1] == '.' && !name[2]))))
> 		return 0;
> 	return !!strchr(name, '/'); /* no slash */
> }

Yeah, that looks right to me.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH] opening files in remote.c should ensure it is opening a file

[Junio C Hamano]

"H.Merijn Brand" <h.m.brand@xs4all.nl> writes:

> HP-UX allows directories to be opened with fopen (path, "r"), which
> will cause some translations that expect to read files, read dirs
> instead. This patch makes sure the two fopen () calls in remote.c
> only open the file if it is a file.

> +static FILE *open_file(char *full_path)
> +{
> +       struct stat st_buf;
> +       if (stat(full_path, &st_buf) || !S_ISREG(st_buf.st_mode))
> +               return NULL;
> +       return (fopen(full_path, "r"));
> +}

Can we make this a platform specific "compat" hack?

It is not fair to force stat() overhead to ports on platforms
that fails fopen() on directories, as I doubt we would ever want
from directory using fopen() anyway.

Re: [PATCH] opening files in remote.c should ensure it is opening a file

[Johannes Schindelin]

Hi,

On Fri, 8 Feb 2008, Junio C Hamano wrote:

> "H.Merijn Brand" <h.m.brand@xs4all.nl> writes:
> 
> > HP-UX allows directories to be opened with fopen (path, "r"), which
> > will cause some translations that expect to read files, read dirs
> > instead. This patch makes sure the two fopen () calls in remote.c
> > only open the file if it is a file.
> 
> > +static FILE *open_file(char *full_path)
> > +{
> > +       struct stat st_buf;
> > +       if (stat(full_path, &st_buf) || !S_ISREG(st_buf.st_mode))
> > +               return NULL;
> > +       return (fopen(full_path, "r"));
> > +}
> 
> Can we make this a platform specific "compat" hack?

You mean something like

#ifdef FOPEN_OPENS_DIRECTORIES
inline static FILE *fopen_compat(const char *path, const char *mode)
{
       struct stat st_buf;
       if (stat(path, &st_buf) || !S_ISREG(st_buf.st_mode))
               return NULL;
       return (fopen(path, mode));
}
#define fopen fopen_compat
#endif

in git-compat-util.h, right?

Yeah, I can see that, even if I think the overhead would not be _that_ 
crucial.  But it is a nice way of fixing _all_ fopen() calls at the same 
time.

Ciao,
Dscho

Re: [PATCH] opening files in remote.c should ensure it is opening a file

[H.Merijn Brand]

On Fri, 08 Feb 2008 12:09:46 -0800, Junio C Hamano <gitster@pobox.com> wrote:

> "H.Merijn Brand" <h.m.brand@xs4all.nl> writes:
> 
> > HP-UX allows directories to be opened with fopen (path, "r"), which
> > will cause some translations that expect to read files, read dirs
> > instead. This patch makes sure the two fopen () calls in remote.c
> > only open the file if it is a file.
> 
> > +static FILE *open_file(char *full_path)
> > +{
> > +       struct stat st_buf;
> > +       if (stat(full_path, &st_buf) || !S_ISREG(st_buf.st_mode))
> > +               return NULL;
> > +       return (fopen(full_path, "r"));
> > +}
> 
> Can we make this a platform specific "compat" hack?
> 
> It is not fair to force stat() overhead to ports on platforms
> that fails fopen() on directories,

The two I patched were in remote.c and do not happen on every file if I
analyzed it correctly, so overhead would be minimal. However, as I read
the rest of the discussion already, your approach to fix all fopen ()
calls at once seems very reasonable.

Can I get the patch when it is submitted?

> as I doubt we would ever want from directory using fopen() anyway.

I didn't check

-- 
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/

Re: [PATCH] opening files in remote.c should ensure it is opening a file

[Morten Welinder]

> +/* Helper function to ensure that we are opening a file and not a directory */
> +static FILE *open_file(char *full_path)
> +{
> +       struct stat st_buf;
> +       if (stat(full_path, &st_buf) || !S_ISREG(st_buf.st_mode))
> +               return NULL;
> +       return (fopen(full_path, "r"));
> +}

That looks wrong.  stat+fopen has a pointless race condition that
open+fstat+fdopen would not have.

Morten

Re: [PATCH] opening files in remote.c should ensure it is opening a file

[Junio C Hamano]

"Morten Welinder" <mwelinder@gmail.com> writes:

>> +/* Helper function to ensure that we are opening a file and not a directory */
>> +static FILE *open_file(char *full_path)
>> +{
>> +       struct stat st_buf;
>> +       if (stat(full_path, &st_buf) || !S_ISREG(st_buf.st_mode))
>> +               return NULL;
>> +       return (fopen(full_path, "r"));
>> +}
>
> That looks wrong.  stat+fopen has a pointless race condition that
> open+fstat+fdopen would not have.

That's true.  How about doing something like this?

 (1) in a new file "compat/gitfopen.c" have this:

	#include "../git-compat-util.h"
	#undef fopen
	FILE *gitfopen(const char *path, const char *mode)
        {
		int fd, flags;
                struct stat st;
        	if (mode[0] == 'w')
                	return fopen(path, mode);
		switch (mode[0]) {
                case 'r': flags = O_RDONLY; break;
                case 'a': flags = O_APPEND; break;
		default:
			errno = EINVAL;
                	return NULL;
		}
		fd = open(path, flags);
		if (fd < 0 || fstat(fd, &st))
                	return NULL;
		if (S_ISDIR(st_buf.st_mode)) {
                	errno = EISDIR;
                        return NULL;
		}
		return fdopen(fd, mode);
	}

  (2) in "git-compat-util.h" have this:

	#ifdef FOPEN_OPENS_DIRECTORIES
        #define fopen(a,b) gitfopen(a,b)
	extern FILE *gitfopen(const char *, const char *);
        #endif

And have Makefile set FOPEN_OPENS_DIRECTORIES on appropriate
platforms.

Re: [PATCH] opening files in remote.c should ensure it is opening a file

[Johannes Schindelin]

Hi,

On Fri, 8 Feb 2008, Junio C Hamano wrote:

> 	#ifdef FOPEN_OPENS_DIRECTORIES

Funny... our emails crossed, and you picked the same name ;-)

Ciao,
Dscho

Re: [PATCH] opening files in remote.c should ensure it is opening a file

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Fri, 8 Feb 2008, Junio C Hamano wrote:
>
>> 	#ifdef FOPEN_OPENS_DIRECTORIES
>
> Funny... our emails crossed, and you picked the same name ;-)

Bad Dscho.

It has been a very well kept secret that Dscho and Junio are one
and the same person, but you just spilled the beans.

;-)

Re: [PATCH] opening files in remote.c should ensure it is opening a file

[Johannes Schindelin]

Hi,

On Fri, 8 Feb 2008, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Fri, 8 Feb 2008, Junio C Hamano wrote:
> >
> >> 	#ifdef FOPEN_OPENS_DIRECTORIES
> >
> > Funny... our emails crossed, and you picked the same name ;-)
> 
> Bad Dscho.
> 
> It has been a very well kept secret that Dscho and Junio are one
> and the same person, but you just spilled the beans.

Shush... left-hemisphere: shut up.

Ciao,
Dscho

P.S.: double ;-)

Re: [PATCH] opening files in remote.c should ensure it is opening a file

[Brandon Casey]

Junio C Hamano wrote:

> And have Makefile set FOPEN_OPENS_DIRECTORIES on appropriate
> platforms.

Which ones _don't_ open directories?

Shouldn't fopen("path_to_some_directory", "r") work?

-brandon

#include <stdio.h>

int main(int argc, char* argv[])
{
    if (!fopen(argv[1], "r")) {
        perror("File open failed");
        return 1;
    }

    puts("File open succeeded.");

    return 0;
}

Re: [PATCH] opening files in remote.c should ensure it is opening a file

[Brandon Casey]

Brandon Casey wrote:
> Junio C Hamano wrote:
> 
>> And have Makefile set FOPEN_OPENS_DIRECTORIES on appropriate
>> platforms.
> 
> Which ones _don't_ open directories?
> 
> Shouldn't fopen("path_to_some_directory", "r") work?

Ok. It's the FOPEN_OPENS_DIRECTORIES term that confused me.

fopen is expected to succeed, even on directories. It's the read
that should fail but is not on HPUX. Obviously we don't want to
change the read.

-brandon

Re: [PATCH] opening files in remote.c should ensure it is opening a file

[Junio C Hamano]

Brandon Casey <casey@nrlssc.navy.mil> writes:

> Junio C Hamano wrote:
>
>> And have Makefile set FOPEN_OPENS_DIRECTORIES on appropriate
>> platforms.
>
> Which ones _don't_ open directories?

Ahh, sorry, of course you are right.  We need to fix the
callers.

Re: [PATCH] opening files in remote.c should ensure it is opening a file

[Brandon Casey]

Junio C Hamano wrote:
> "Morten Welinder" <mwelinder@gmail.com> writes:
> 
>>> +/* Helper function to ensure that we are opening a file and not a directory */
>>> +static FILE *open_file(char *full_path)
>>> +{
>>> +       struct stat st_buf;
>>> +       if (stat(full_path, &st_buf) || !S_ISREG(st_buf.st_mode))
>>> +               return NULL;
>>> +       return (fopen(full_path, "r"));
>>> +}
>> That looks wrong.  stat+fopen has a pointless race condition that
>> open+fstat+fdopen would not have.
> 
> That's true.  How about doing something like this?
> 
>  (1) in a new file "compat/gitfopen.c" have this:
> 
> 	#include "../git-compat-util.h"
> 	#undef fopen
> 	FILE *gitfopen(const char *path, const char *mode)
>         {
> 		int fd, flags;
>                 struct stat st;
>         	if (mode[0] == 'w')
>                 	return fopen(path, mode);
> 		switch (mode[0]) {
>                 case 'r': flags = O_RDONLY; break;
>                 case 'a': flags = O_APPEND; break;
> 		default:
> 			errno = EINVAL;
>                 	return NULL;
> 		}
> 		fd = open(path, flags);
> 		if (fd < 0 || fstat(fd, &st))
>                 	return NULL;
> 		if (S_ISDIR(st_buf.st_mode)) {
>                 	errno = EISDIR;
>                         return NULL;
> 		}
> 		return fdopen(fd, mode);
> 	}

Can we use fileno()? Something like:

FILE *gitfopen(const char *path, const char *mode)
{   
        FILE *fp;
        struct stat st;

        if (strpbrk(mode, "wa"))
                return fopen(path, mode);

        if (!(fp = fopen(path, mode)))
                return NULL;

        if (fstat(fileno(fp), &st)) {
                fclose(fp);
                return NULL;
        }

        if (S_ISDIR(st.st_mode)) {
                fclose(fp);
                errno = EISDIR;
                return NULL;
        }

        return fp;
}   

-brandon

[PATCH] Add compat/fopen.c which returns NULL on attempt to open directory

[Brandon Casey]

Some systems do not fail as expected when fread et al. are called on
a directory stream. Replace fopen on such systems which will fail
when the supplied path is a directory.

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---
 Makefile          |    7 +++++++
 compat/fopen.c    |   26 ++++++++++++++++++++++++++
 git-compat-util.h |    5 +++++
 3 files changed, 38 insertions(+), 0 deletions(-)
 create mode 100644 compat/fopen.c

diff --git a/Makefile b/Makefile
index 92341c4..debfc23 100644
--- a/Makefile
+++ b/Makefile
@@ -3,6 +3,9 @@ all::
 
 # Define V=1 to have a more verbose compile.
 #
+# Define FREAD_READS_DIRECTORIES if your are on a system which succeeds
+# when attempting to read from an fopen'ed directory.
+#
 # Define NO_OPENSSL environment variable if you do not have OpenSSL.
 # This also implies MOZILLA_SHA1.
 #
@@ -618,6 +621,10 @@ endif
 ifdef NO_C99_FORMAT
 	BASIC_CFLAGS += -DNO_C99_FORMAT
 endif
+ifdef FREAD_READS_DIRECTORIES
+	COMPAT_CFLAGS += -DFREAD_READS_DIRECTORIES
+	COMPAT_OBJS += compat/fopen.o
+endif
 ifdef NO_SYMLINK_HEAD
 	BASIC_CFLAGS += -DNO_SYMLINK_HEAD
 endif
diff --git a/compat/fopen.c b/compat/fopen.c
new file mode 100644
index 0000000..ccb9e89
--- /dev/null
+++ b/compat/fopen.c
@@ -0,0 +1,26 @@
+#include "../git-compat-util.h"
+#undef fopen
+FILE *git_fopen(const char *path, const char *mode)
+{
+	FILE *fp;
+	struct stat st;
+
+	if (mode[0] == 'w' || mode[0] == 'a')
+		return fopen(path, mode);
+
+	if (!(fp = fopen(path, mode)))
+		return NULL;
+
+	if (fstat(fileno(fp), &st)) {
+		fclose(fp);
+		return NULL;
+	}
+
+	if (S_ISDIR(st.st_mode)) {
+		fclose(fp);
+		errno = EISDIR;
+		return NULL;
+	}
+
+	return fp;
+}
diff --git a/git-compat-util.h b/git-compat-util.h
index 4df90cb..46d5e93 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -204,6 +204,11 @@ void *gitmemmem(const void *haystack, size_t haystacklen,
                 const void *needle, size_t needlelen);
 #endif
 
+#ifdef FREAD_READS_DIRECTORIES
+#define fopen(a,b) git_fopen(a,b)
+extern FILE *git_fopen(const char*, const char*);
+#endif
+
 #ifdef __GLIBC_PREREQ
 #if __GLIBC_PREREQ(2, 1)
 #define HAVE_STRCHRNUL
-- 
1.5.4.26.g5ef4da

Re: [PATCH] Add compat/fopen.c which returns NULL on attempt to open directory

[H.Merijn Brand]

On Fri, 08 Feb 2008 20:32:47 -0600, Brandon Casey <casey@nrlssc.navy.mil>
wrote:

> Some systems do not fail as expected when fread et al. are called on
> a directory stream. Replace fopen on such systems which will fail
> when the supplied path is a directory.

I applied this patch instead of mine, and added the Makefile define
Harder to trace, as it is not issuing error messages, but could this
success^Wfailure be related?

/pro/3gl/LINUX/git-1.5.4.rc5 103 > cat t/t5701-clone-local.sh.err
*   ok 1: preparing origin repository
*   ok 2: local clone without .git suffix
*   ok 3: local clone with .git suffix
*   ok 4: local clone from x
* FAIL 5: local clone from x.git that does not exist

                cd "$D" &&
                if git clone -l -s x.git z
                then
                        echo "Oops, should have failed"
                        false
                else
                        echo happy
                fi

*   ok 6: With -no-hardlinks, local will make a copy
*   ok 7: Even without -l, local will make a hardlink
* failed 1 among 7 test(s)

Any hints in where to start digging?

> Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
> ---
>  Makefile          |    7 +++++++
>  compat/fopen.c    |   26 ++++++++++++++++++++++++++
>  git-compat-util.h |    5 +++++
>  3 files changed, 38 insertions(+), 0 deletions(-)
>  create mode 100644 compat/fopen.c
> 
> diff --git a/Makefile b/Makefile
> index 92341c4..debfc23 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -3,6 +3,9 @@ all::
>  
>  # Define V=1 to have a more verbose compile.
>  #
> +# Define FREAD_READS_DIRECTORIES if your are on a system which succeeds
> +# when attempting to read from an fopen'ed directory.
> +#
>  # Define NO_OPENSSL environment variable if you do not have OpenSSL.
>  # This also implies MOZILLA_SHA1.
>  #
> @@ -618,6 +621,10 @@ endif
>  ifdef NO_C99_FORMAT
>  	BASIC_CFLAGS += -DNO_C99_FORMAT
>  endif
> +ifdef FREAD_READS_DIRECTORIES
> +	COMPAT_CFLAGS += -DFREAD_READS_DIRECTORIES
> +	COMPAT_OBJS += compat/fopen.o
> +endif
>  ifdef NO_SYMLINK_HEAD
>  	BASIC_CFLAGS += -DNO_SYMLINK_HEAD
>  endif
> diff --git a/compat/fopen.c b/compat/fopen.c
> new file mode 100644
> index 0000000..ccb9e89
> --- /dev/null
> +++ b/compat/fopen.c
> @@ -0,0 +1,26 @@
> +#include "../git-compat-util.h"
> +#undef fopen
> +FILE *git_fopen(const char *path, const char *mode)
> +{
> +	FILE *fp;
> +	struct stat st;
> +
> +	if (mode[0] == 'w' || mode[0] == 'a')
> +		return fopen(path, mode);
> +
> +	if (!(fp = fopen(path, mode)))
> +		return NULL;
> +
> +	if (fstat(fileno(fp), &st)) {
> +		fclose(fp);
> +		return NULL;
> +	}
> +
> +	if (S_ISDIR(st.st_mode)) {
> +		fclose(fp);
> +		errno = EISDIR;
> +		return NULL;
> +	}
> +
> +	return fp;
> +}
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 4df90cb..46d5e93 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -204,6 +204,11 @@ void *gitmemmem(const void *haystack, size_t haystacklen,
>                  const void *needle, size_t needlelen);
>  #endif
>  
> +#ifdef FREAD_READS_DIRECTORIES
> +#define fopen(a,b) git_fopen(a,b)
> +extern FILE *git_fopen(const char*, const char*);
> +#endif
> +
>  #ifdef __GLIBC_PREREQ
>  #if __GLIBC_PREREQ(2, 1)
>  #define HAVE_STRCHRNUL


-- 
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/

Re: [PATCH] Add compat/fopen.c which returns NULL on attempt to open directory

[H.Merijn Brand]

On Mon, 11 Feb 2008 10:29:50 +0100, "H.Merijn Brand" <h.m.brand@xs4all.nl>
wrote:

> On Fri, 08 Feb 2008 20:32:47 -0600, Brandon Casey <casey@nrlssc.navy.mil>
> wrote:
> 
> > Some systems do not fail as expected when fread et al. are called on
> > a directory stream. Replace fopen on such systems which will fail
> > when the supplied path is a directory.
> 
> I applied this patch instead of mine, and added the Makefile define
> Harder to trace, as it is not issuing error messages, but could this
> success^Wfailure be related?

No, it is not. Some shell weirdness. This fixes it. Don't know off-hand
if it is portable enough

diff -pur a/t/t5701-clone-local.sh b/t/t5701-clone-local.sh
--- a/t/t5701-clone-local.sh  2008-02-02 05:09:01 +0100
+++ b/t/t5701-clone-local.sh  2008-02-11 11:13:26 +0100
@@ -37,8 +37,8 @@ test_expect_success 'local clone from x'

 test_expect_success 'local clone from x.git that does not exist' '
        cd "$D" &&
-       if git clone -l -s x.git z
-       then
+       git clone -l -s x.git z
+       if $? ; then
                echo "Oops, should have failed"
                false
        else

-- 
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/

Re: [PATCH] Add compat/fopen.c which returns NULL on attempt to open directory

[Junio C Hamano]

"H.Merijn Brand" <h.m.brand@xs4all.nl> writes:

> No, it is not. Some shell weirdness. This fixes it. Don't know off-hand
> if it is portable enough
>
> diff -pur a/t/t5701-clone-local.sh b/t/t5701-clone-local.sh
> --- a/t/t5701-clone-local.sh  2008-02-02 05:09:01 +0100
> +++ b/t/t5701-clone-local.sh  2008-02-11 11:13:26 +0100
> @@ -37,8 +37,8 @@ test_expect_success 'local clone from x'
>
>  test_expect_success 'local clone from x.git that does not exist' '
>         cd "$D" &&
> -       if git clone -l -s x.git z
> -       then
> +       git clone -l -s x.git z
> +       if $? ; then
>                 echo "Oops, should have failed"
>                 false
>         else

I think your "git clone" is broken and I strongly suspect it is
not your shell (at least the "if" construct in the test).

What's 

	if $?; then

In sane shells, I think this tries to execute 0 or perhaps 124
or whatever the error code from clone as if it was the name of a
command, which would most likely fail and would not take "then"
part (which reports the error).  It did not fix, but just made
it ignore the error from "git clone".

If it were

	if test $? != 0
        then

it would have made a bit more sense.

And if (this is a big "if" as I doubt any shell is so broken)
these two are equivalent to your shell, then I do not think it
is portable at all.

Re: [PATCH] Add compat/fopen.c which returns NULL on attempt to open directory

[H.Merijn Brand]

On Mon, 11 Feb 2008 16:20:05 -0800, Junio C Hamano <gitster@pobox.com> wrote:

> "H.Merijn Brand" <h.m.brand@xs4all.nl> writes:
> 
> > No, it is not. Some shell weirdness. This fixes it. Don't know off-hand
> > if it is portable enough
> >
> > diff -pur a/t/t5701-clone-local.sh b/t/t5701-clone-local.sh
> > --- a/t/t5701-clone-local.sh  2008-02-02 05:09:01 +0100
> > +++ b/t/t5701-clone-local.sh  2008-02-11 11:13:26 +0100
> > @@ -37,8 +37,8 @@ test_expect_success 'local clone from x'
> >
> >  test_expect_success 'local clone from x.git that does not exist' '
> >         cd "$D" &&
> > -       if git clone -l -s x.git z
> > -       then
> > +       git clone -l -s x.git z
> > +       if $? ; then
> >                 echo "Oops, should have failed"
> >                 false
> >         else
> 
> I think your "git clone" is broken and I strongly suspect it is
> not your shell (at least the "if" construct in the test).

of course it should have been 'if test $?' and as $? is erroneously
equal to 0 in this case, this snippet doesn't matter

'git clone' is calling 'cit-clone' which is a shell script, that does
exit 1 in the function die:
--8<---
die() {
	echo >&2 "$@"
	exit 1
}
-->8---

but somehow that exit code gets lost

> What's 
> 
> 	if $?; then
> 
> In sane shells, I think this tries to execute 0 or perhaps 124
> or whatever the error code from clone as if it was the name of a
> command, which would most likely fail and would not take "then"
> part (which reports the error).  It did not fix, but just made
> it ignore the error from "git clone".
> 
> If it were
> 
> 	if test $? != 0
>         then
> 
> it would have made a bit more sense.
> 
> And if (this is a big "if" as I doubt any shell is so broken)
> these two are equivalent to your shell, then I do not think it
> is portable at all.

-- 
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/