# [PATCH] opening files in remote.c should ensure it is opening a file 27 messages from 2008-02-08 to 2008-02-18. Participants: H.Merijn Brand, Mike Ralphson, Junio C Hamano, Morten Welinder, Daniel Barkalow, Johannes Schindelin, Brandon Casey. Thread: https://gitlist.dev/t/11971 ## H.Merijn Brand, 2008-02-08 16:46 Subject: [PATCH] opening files in remote.c should ensure it is opening a file Message-ID: <20080208174654.2e9e679c@pc09.procura.nl> URL: https://gitlist.dev/e/20080208174654.2e9e679c%40pc09.procura.nl ``` 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 --- 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/ ``` ## Mike Ralphson, 2008-02-08 17:25 Subject: Re: [PATCH] opening files in remote.c should ensure it is opening a file Message-ID: URL: https://gitlist.dev/e/e2b179460802080925s61270036q81896010c76236ae%40mail.gmail.com In-Reply-To: <20080208174654.2e9e679c@pc09.procura.nl> ``` On Feb 8, 2008 4:46 PM, H.Merijn Brand 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 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 Your other fix there [- if (!strbuf_avail(sb)) / + if (strbuf_avail(sb) < 64) ] is, guess what, also required on AIX. Thanks again. ``` ## H.Merijn Brand, 2008-02-08 20:04 Subject: Re: [PATCH] opening files in remote.c should ensure it is opening a file Message-ID: <20080208210447.289022b6@pc09.procura.nl> URL: https://gitlist.dev/e/20080208210447.289022b6%40pc09.procura.nl In-Reply-To: ``` On Fri, 8 Feb 2008 17:25:52 +0000, "Mike Ralphson" wrote: > On Feb 8, 2008 4:46 PM, H.Merijn Brand 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 > > 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 > > 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/ ``` ## Junio C Hamano, 2008-02-08 20:09 Subject: Re: [PATCH] opening files in remote.c should ensure it is opening a file Message-ID: <7vhcgjjjlh.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vhcgjjjlh.fsf%40gitster.siamese.dyndns.org In-Reply-To: <20080208174654.2e9e679c@pc09.procura.nl> ``` "H.Merijn Brand" 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. ``` ## Morten Welinder, 2008-02-08 20:15 Subject: Re: [PATCH] opening files in remote.c should ensure it is opening a file Message-ID: <118833cc0802081215t380587f6w7b5c0aba66a55799@mail.gmail.com> URL: https://gitlist.dev/e/118833cc0802081215t380587f6w7b5c0aba66a55799%40mail.gmail.com In-Reply-To: <20080208174654.2e9e679c@pc09.procura.nl> ``` > +/* 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 ``` ## Junio C Hamano, 2008-02-08 20:33 Subject: Re: [PATCH] opening files in remote.c should ensure it is opening a file Message-ID: <7v8x1vjiic.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7v8x1vjiic.fsf%40gitster.siamese.dyndns.org In-Reply-To: <118833cc0802081215t380587f6w7b5c0aba66a55799@mail.gmail.com> ``` "Morten Welinder" 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. ``` ## Daniel Barkalow, 2008-02-08 20:36 Subject: Re: [PATCH] opening files in remote.c should ensure it is opening a file Message-ID: URL: https://gitlist.dev/e/alpine.LNX.1.00.0802081526350.13593%40iabervon.org In-Reply-To: ``` On Fri, 8 Feb 2008, Mike Ralphson wrote: > On Feb 8, 2008 4:46 PM, H.Merijn Brand 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 > > 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) ``` ## Johannes Schindelin, 2008-02-08 20:38 Subject: Re: [PATCH] opening files in remote.c should ensure it is opening a file Message-ID: URL: https://gitlist.dev/e/alpine.LSU.1.00.0802082035460.11591%40racer.site In-Reply-To: <7vhcgjjjlh.fsf@gitster.siamese.dyndns.org> ``` Hi, On Fri, 8 Feb 2008, Junio C Hamano wrote: > "H.Merijn Brand" 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 ``` ## Johannes Schindelin, 2008-02-08 20:40 Subject: Re: [PATCH] opening files in remote.c should ensure it is opening a file Message-ID: URL: https://gitlist.dev/e/alpine.LSU.1.00.0802082040010.11591%40racer.site In-Reply-To: <7v8x1vjiic.fsf@gitster.siamese.dyndns.org> ``` 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 ``` ## Johannes Schindelin, 2008-02-08 20:44 Subject: Re: [PATCH] opening files in remote.c should ensure it is opening a file Message-ID: URL: https://gitlist.dev/e/alpine.LSU.1.00.0802082042440.11591%40racer.site In-Reply-To: ``` 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 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 > > > > 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 . " was originally the only way to merge a local branch, and it is still valid. Ciao, Dscho ``` ## Brandon Casey, 2008-02-08 20:58 Subject: Re: [PATCH] opening files in remote.c should ensure it is opening a file Message-ID: <47ACC261.6060404@nrlssc.navy.mil> URL: https://gitlist.dev/e/47ACC261.6060404%40nrlssc.navy.mil In-Reply-To: <7v8x1vjiic.fsf@gitster.siamese.dyndns.org> ``` 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 int main(int argc, char* argv[]) { if (!fopen(argv[1], "r")) { perror("File open failed"); return 1; } puts("File open succeeded."); return 0; } ``` ## Brandon Casey, 2008-02-08 21:14 Subject: Re: [PATCH] opening files in remote.c should ensure it is opening a file Message-ID: <47ACC64E.7040102@nrlssc.navy.mil> URL: https://gitlist.dev/e/47ACC64E.7040102%40nrlssc.navy.mil In-Reply-To: <47ACC261.6060404@nrlssc.navy.mil> ``` 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 ``` ## Junio C Hamano, 2008-02-08 21:19 Subject: Re: [PATCH] opening files in remote.c should ensure it is opening a file Message-ID: <7vwspfi1t4.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vwspfi1t4.fsf%40gitster.siamese.dyndns.org In-Reply-To: ``` Johannes Schindelin 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. ;-) ``` ## Johannes Schindelin, 2008-02-08 21:47 Subject: Re: [PATCH] opening files in remote.c should ensure it is opening a file Message-ID: URL: https://gitlist.dev/e/alpine.LSU.1.00.0802082146400.11591%40racer.site In-Reply-To: <7vwspfi1t4.fsf@gitster.siamese.dyndns.org> ``` Hi, On Fri, 8 Feb 2008, Junio C Hamano wrote: > Johannes Schindelin 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 ;-) ``` ## Brandon Casey, 2008-02-09 01:20 Subject: Re: [PATCH] opening files in remote.c should ensure it is opening a file Message-ID: <47ACFFD9.2030705@nrlssc.navy.mil> URL: https://gitlist.dev/e/47ACFFD9.2030705%40nrlssc.navy.mil In-Reply-To: <7v8x1vjiic.fsf@gitster.siamese.dyndns.org> ``` Junio C Hamano wrote: > "Morten Welinder" 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 ``` ## Brandon Casey, 2008-02-09 02:32 Subject: [PATCH] Add compat/fopen.c which returns NULL on attempt to open directory Message-ID: <47AD10CF.1040207@nrlssc.navy.mil> URL: https://gitlist.dev/e/47AD10CF.1040207%40nrlssc.navy.mil In-Reply-To: <47ACFFD9.2030705@nrlssc.navy.mil> ``` 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 --- 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 ``` ## Junio C Hamano, 2008-02-09 05:12 Subject: Re: [PATCH] opening files in remote.c should ensure it is opening a file Message-ID: <7vzluag1bi.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vzluag1bi.fsf%40gitster.siamese.dyndns.org In-Reply-To: <47ACC261.6060404@nrlssc.navy.mil> ``` Brandon Casey 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. ``` ## Junio C Hamano, 2008-02-09 05:27 Subject: Re: [PATCH] opening files in remote.c should ensure it is opening a file Message-ID: <7vk5leg0nj.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vk5leg0nj.fsf%40gitster.siamese.dyndns.org In-Reply-To: ``` Daniel Barkalow 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 */ } ``` ## Daniel Barkalow, 2008-02-09 05:54 Subject: Re: [PATCH] opening files in remote.c should ensure it is opening a file Message-ID: URL: https://gitlist.dev/e/alpine.LNX.1.00.0802090053420.13593%40iabervon.org In-Reply-To: <7vk5leg0nj.fsf@gitster.siamese.dyndns.org> ``` On Fri, 8 Feb 2008, Junio C Hamano wrote: > Daniel Barkalow 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* ``` ## H.Merijn Brand, 2008-02-09 10:03 Subject: Re: [PATCH] opening files in remote.c should ensure it is opening a file Message-ID: <20080209110341.2337a19d@pc09.procura.nl> URL: https://gitlist.dev/e/20080209110341.2337a19d%40pc09.procura.nl In-Reply-To: <7vhcgjjjlh.fsf@gitster.siamese.dyndns.org> ``` On Fri, 08 Feb 2008 12:09:46 -0800, Junio C Hamano wrote: > "H.Merijn Brand" 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/ ``` ## H.Merijn Brand, 2008-02-11 09:29 Subject: Re: [PATCH] Add compat/fopen.c which returns NULL on attempt to open directory Message-ID: <20080211102950.122ba93d@pc09.procura.nl> URL: https://gitlist.dev/e/20080211102950.122ba93d%40pc09.procura.nl In-Reply-To: <47AD10CF.1040207@nrlssc.navy.mil> ``` On Fri, 08 Feb 2008 20:32:47 -0600, Brandon Casey 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 > --- > 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/ ``` ## H.Merijn Brand, 2008-02-11 10:15 Subject: Re: [PATCH] Add compat/fopen.c which returns NULL on attempt to open directory Message-ID: <20080211111537.2bf47448@pc09.procura.nl> URL: https://gitlist.dev/e/20080211111537.2bf47448%40pc09.procura.nl In-Reply-To: <20080211102950.122ba93d@pc09.procura.nl> ``` On Mon, 11 Feb 2008 10:29:50 +0100, "H.Merijn Brand" wrote: > On Fri, 08 Feb 2008 20:32:47 -0600, Brandon Casey > 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/ ``` ## Junio C Hamano, 2008-02-12 00:20 Subject: Re: [PATCH] Add compat/fopen.c which returns NULL on attempt to open directory Message-ID: <7v8x1r6n62.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7v8x1r6n62.fsf%40gitster.siamese.dyndns.org In-Reply-To: <20080211111537.2bf47448@pc09.procura.nl> ``` "H.Merijn Brand" 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. ``` ## H.Merijn Brand, 2008-02-12 15:27 Subject: Re: [PATCH] Add compat/fopen.c which returns NULL on attempt to open directory Message-ID: <20080212162722.1d98e05d@pc09.procura.nl> URL: https://gitlist.dev/e/20080212162722.1d98e05d%40pc09.procura.nl In-Reply-To: <7v8x1r6n62.fsf@gitster.siamese.dyndns.org> ``` On Mon, 11 Feb 2008 16:20:05 -0800, Junio C Hamano wrote: > "H.Merijn Brand" 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/ ``` ## H.Merijn Brand, 2008-02-18 09:10 Subject: Re: [PATCH] opening files in remote.c should ensure it is opening a file Message-ID: <20080218101026.6098667f@pc09.procura.nl> URL: https://gitlist.dev/e/20080218101026.6098667f%40pc09.procura.nl In-Reply-To: <20080208210447.289022b6@pc09.procura.nl> ``` On Fri, 8 Feb 2008 21:04:47 +0100, "H.Merijn Brand" 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/ ``` ## Junio C Hamano, 2008-02-18 09:30 Subject: Re: [PATCH] opening files in remote.c should ensure it is opening a file Message-ID: <7v3arqr4qo.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7v3arqr4qo.fsf%40gitster.siamese.dyndns.org In-Reply-To: <20080218101026.6098667f@pc09.procura.nl> ``` "H.Merijn Brand" 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. ``` ## H.Merijn Brand, 2008-02-18 11:31 Subject: Re: [PATCH] opening files in remote.c should ensure it is opening a file Message-ID: <20080218123157.17c91f20@pc09.procura.nl> URL: https://gitlist.dev/e/20080218123157.17c91f20%40pc09.procura.nl In-Reply-To: <7v3arqr4qo.fsf@gitster.siamese.dyndns.org> ``` On Mon, 18 Feb 2008 01:30:39 -0800, Junio C Hamano wrote: > "H.Merijn Brand" 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/ ```