{"thread":{"id":"11969","subject":"read_branches_file ()","startedAt":"2008-02-08T15:50:08Z","lastAt":"2008-02-11T10:17:41Z","messageCount":7,"participants":["H.Merijn Brand","Johannes Schindelin","Daniel Barkalow","Andreas Ericsson"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"67957","messageId":"20080208165008.52630d36@pc09.procura.nl","threadId":"11969","inReplyTo":null,"subject":"read_branches_file ()","fromName":"H.Merijn Brand","fromEmail":"h.m.brand@xs4all.nl","sentAt":"2008-02-08T15:50:08Z","receivedAt":"2008-02-08T15:50:08Z","isPatch":false,"sender":{"key":"h.m.brand@xs4all.nl","avatar":"https://gravatar.com/avatar/5b8f83ee35c427a646cbea3b104346e00ab3663b99bbf435cddeb75cd4b3857b?d=mp&s=160"},"body":"I'm (again) trying to port git-1.5.4 to HP-UX, and I've already got rather\nfar, but I'm hitting some stuff I cannot explain.\n\nt5405-send-pack-rewind.sh fails the 'git fetch .. master:master' part in\nthe setup, as deep down, read_branches_file () is called with in remote\nthe name \"..\".\n\nThe file that it tries to open using git_path () is \".git/branches/..\"\nThat is weird. That is not a file, but a dir. \"../.git/branches\" would\nbe more logical, but whatever. HP-UX 11.00 will gladly return a valid\nFILE * for opening a directory with fopen (), which, when read, will\nreturn anything but what is expected. So, maybe read_branches_file ()\nshould be protected against opening anything but files. Maybe with some\nstat () and S_ISREG ()'s.\n\nOr has something gone wrong earlier on?\n\nIn my case, the returned url is 'l', which cannot be opened:\n\nfatal: 'l': unable to chdir or not a git archive\nfatal: The remote end hung up unexpectedly\n\nwhich is cast from upload-pack.c:main ()\n\n-- \nH.Merijn Brand Amsterdam Perl Mongers (http://amsterdam.pm.org/)\nusing & porting perl 5.6.2, 5.8.x, 5.10.x on HP-UX 10.20, 11.00, 11.11,\n& 11.23, SuSE 10.1 & 10.2, AIX 5.2, and Cygwin. http://qa.perl.org\nhttp://mirrors.develooper.com/hpux/ http://www.test-smoke.org\n http://www.goldmark.org/jeff/stupid-disclaimers/\n"},{"id":"67958","messageId":"20080208170305.069d43d2@pc09.procura.nl","threadId":"11969","inReplyTo":"20080208165008.52630d36@pc09.procura.nl","subject":"Re: read_branches_file ()","fromName":"H.Merijn Brand","fromEmail":"h.m.brand@xs4all.nl","sentAt":"2008-02-08T16:03:05Z","receivedAt":"2008-02-08T16:03:05Z","isPatch":false,"sender":{"key":"h.m.brand@xs4all.nl","avatar":"https://gravatar.com/avatar/5b8f83ee35c427a646cbea3b104346e00ab3663b99bbf435cddeb75cd4b3857b?d=mp&s=160"},"body":"On Fri, 8 Feb 2008 16:50:08 +0100, \"H.Merijn Brand\" \nwrote:\n\n> I'm (again) trying to port git-1.5.4 to HP-UX, and I've already got rather\n> far, but I'm hitting some stuff I cannot explain.\n> \n> t5405-send-pack-rewind.sh fails the 'git fetch .. master:master' part in\n> the setup, as deep down, read_branches_file () is called with in remote\n> the name \"..\".\n> \n> The file that it tries to open using git_path () is \".git/branches/..\"\n> That is weird. That is not a file, but a dir. \"../.git/branches\" would\n> be more logical, but whatever. HP-UX 11.00 will gladly return a valid\n> FILE * for opening a directory with fopen (), which, when read, will\n> return anything but what is expected. So, maybe read_branches_file ()\n> should be protected against opening anything but files. Maybe with some\n> stat () and S_ISREG ()'s.\n\nSomething like this seems so `fix' this specific problem.\nFeel free to take another approach, t5405 now passes\n\n--8<--- remote.c.diff\n--- remote.c.org 2008-01-27 09:04:18 +0100\n+++ remote.c 2008-02-08 17:01:09 +0100\n@@ -1,6 +1,7 @@\n #include \"cache.h\"\n #include \"remote.h\"\n #include \"refs.h\"\n+#include \n\n static struct remote **remotes;\n static int allocated_remotes;\n@@ -173,11 +174,15 @@ static void read_branches_file(struct re\n char *frag;\n char *branch;\n int n = slash ? slash - remote->name : 1000;\n- FILE *f = fopen(git_path(\"branches/%.*s\", n, remote->name), \"r\");\n+ char *gp = git_path (\"branches/%.*s\", n, remote->name);\n+ struct stat st_buf;\n+ FILE *f;\n char *s, *p;\n int len;\n\n- if (!f)\n+ if (stat (gp, &st_buf) || S_ISDIR (st_buf.st_mode))\n+ return;\n+ if (!(f = fopen(gp, \"r\")))\n return;\n s = fgets(buffer, BUF_SIZE, f);\n fclose(f);\n-->8---\n\n> Or has something gone wrong earlier on?\n> \n> In my case, the returned url is 'l', which cannot be opened:\n> \n> fatal: 'l': unable to chdir or not a git archive\n> fatal: The remote end hung up unexpectedly\n> \n> which is cast from upload-pack.c:main ()\n\n-- \nH.Merijn Brand Amsterdam Perl Mongers (http://amsterdam.pm.org/)\nusing & porting perl 5.6.2, 5.8.x, 5.10.x on HP-UX 10.20, 11.00, 11.11,\n& 11.23, SuSE 10.1 & 10.2, AIX 5.2, and Cygwin. http://qa.perl.org\nhttp://mirrors.develooper.com/hpux/ http://www.test-smoke.org\n http://www.goldmark.org/jeff/stupid-disclaimers/\n"},{"id":"67960","messageId":"alpine.LSU.1.00.0802081618210.11591@racer.site","threadId":"11969","inReplyTo":"20080208170305.069d43d2@pc09.procura.nl","subject":"Re: read_branches_file ()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-02-08T16:22:45Z","receivedAt":"2008-02-08T16:22:45Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 8 Feb 2008, H.Merijn Brand wrote:\n\n> --8<--- remote.c.diff\n> --- remote.c.org 2008-01-27 09:04:18 +0100\n> +++ remote.c 2008-02-08 17:01:09 +0100\n> @@ -1,6 +1,7 @@\n> #include \"cache.h\"\n> #include \"remote.h\"\n> #include \"refs.h\"\n> +#include \n\nThis should not be necessary; we include all system headers in cache.h.\n\n> @@ -173,11 +174,15 @@ static void read_branches_file(struct re\n> char *frag;\n> char *branch;\n> int n = slash ? slash - remote->name : 1000;\n> - FILE *f = fopen(git_path(\"branches/%.*s\", n, remote->name), \"r\");\n> + char *gp = git_path (\"branches/%.*s\", n, remote->name);\n\nPlease use a more descriptive variable name, such as \"branches_file\" or \n\"branches_path\".\n\nAlso, we only leave a space after operators like \"for\", \"while\", but not \nafter function names.\n\n> + if (stat (gp, &st_buf) || S_ISDIR (st_buf.st_mode))\n\nAgain, please remove the spaces after \"stat\" and \"S_ISDIR\".\n\nOther than that, the patch looks obviously correct: please resubmit with a \nnice commit message and a sign-off.\n\nThanks,\nDscho\n"},{"id":"67969","messageId":"20080208174924.24203ff3@pc09.procura.nl","threadId":"11969","inReplyTo":"alpine.LSU.1.00.0802081618210.11591@racer.site","subject":"Re: read_branches_file ()","fromName":"H.Merijn Brand","fromEmail":"h.m.brand@xs4all.nl","sentAt":"2008-02-08T16:49:24Z","receivedAt":"2008-02-08T16:49:24Z","isPatch":false,"sender":{"key":"h.m.brand@xs4all.nl","avatar":"https://gravatar.com/avatar/5b8f83ee35c427a646cbea3b104346e00ab3663b99bbf435cddeb75cd4b3857b?d=mp&s=160"},"body":"On Fri, 8 Feb 2008 16:22:45 +0000 (GMT), Johannes Schindelin\n wrote:\n\n> Hi,\n> \n> On Fri, 8 Feb 2008, H.Merijn Brand wrote:\n> \n> > --8<--- remote.c.diff\n> > --- remote.c.org 2008-01-27 09:04:18 +0100\n> > +++ remote.c 2008-02-08 17:01:09 +0100\n> > @@ -1,6 +1,7 @@\n> > #include \"cache.h\"\n> > #include \"remote.h\"\n> > #include \"refs.h\"\n> > +#include \n> \n> This should not be necessary; we include all system headers in cache.h.\n\ndropped\n\n> > @@ -173,11 +174,15 @@ static void read_branches_file(struct re\n> > char *frag;\n> > char *branch;\n> > int n = slash ? slash - remote->name : 1000;\n> > - FILE *f = fopen(git_path(\"branches/%.*s\", n, remote->name), \"r\");\n> > + char *gp = git_path (\"branches/%.*s\", n, remote->name);\n> \n> Please use a more descriptive variable name, such as \"branches_file\" or \n> \"branches_path\".\n\ntook another approach, as that also addresses the other fopen () call\n\n> Also, we only leave a space after operators like \"for\", \"while\", but not \n> after function names.\n\nThis patch was not sent to be applied as-is, only as a proof-of-concept\nNot that I agree to the layout/indentation, the new patch is sent trying\nto follow what you use.\n\n> > + if (stat (gp, &st_buf) || S_ISDIR (st_buf.st_mode))\n> \n> Again, please remove the spaces after \"stat\" and \"S_ISDIR\".\n\nyou said please :)\n\n> Other than that, the patch looks obviously correct: please resubmit with a \n> nice commit message and a sign-off.\n\nDone\n\n-- \nH.Merijn Brand Amsterdam Perl Mongers (http://amsterdam.pm.org/)\nusing & porting perl 5.6.2, 5.8.x, 5.10.x on HP-UX 10.20, 11.00, 11.11,\n& 11.23, SuSE 10.1 & 10.2, AIX 5.2, and Cygwin. http://qa.perl.org\nhttp://mirrors.develooper.com/hpux/ http://www.test-smoke.org\n http://www.goldmark.org/jeff/stupid-disclaimers/\n"},{"id":"67972","messageId":"alpine.LNX.1.00.0802081213340.13593@iabervon.org","threadId":"11969","inReplyTo":"20080208165008.52630d36@pc09.procura.nl","subject":"Re: read_branches_file ()","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-02-08T17:18:50Z","receivedAt":"2008-02-08T17:18:50Z","isPatch":false,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Fri, 8 Feb 2008, H.Merijn Brand wrote:\n\n> I'm (again) trying to port git-1.5.4 to HP-UX, and I've already got rather\n> far, but I'm hitting some stuff I cannot explain.\n> \n> t5405-send-pack-rewind.sh fails the 'git fetch .. master:master' part in\n> the setup, as deep down, read_branches_file () is called with in remote\n> the name \"..\".\n> \n> The file that it tries to open using git_path () is \".git/branches/..\"\n> That is weird. That is not a file, but a dir. \"../.git/branches\" would\n> be more logical, but whatever. HP-UX 11.00 will gladly return a valid\n> FILE * for opening a directory with fopen (), which, when read, will\n> return anything but what is expected. So, maybe read_branches_file ()\n> should be protected against opening anything but files. Maybe with some\n> stat () and S_ISREG ()'s.\n> \n> Or has something gone wrong earlier on?\n\nIt's doing this because it thinks you might have a remote configured with \nthe name \"..\", which should probably be blocked at a higher level, if only \nfor general sanity.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"68354","messageId":"47B01CD4.4040004@op5.se","threadId":"11969","inReplyTo":"20080208170305.069d43d2@pc09.procura.nl","subject":"Re: read_branches_file ()","fromName":"Andreas Ericsson","fromEmail":"ae@op5.se","sentAt":"2008-02-11T10:00:52Z","receivedAt":"2008-02-11T10:00:52Z","isPatch":false,"sender":{"key":"ae@op5.se","avatar":"https://gravatar.com/avatar/426e89595c75a8f5252dd0c989e5fabe5bcac616e68557427ad9aef6b0ca342a?d=mp&s=160"},"body":"H.Merijn Brand wrote:\n> - if (!f)\n> + if (stat (gp, &st_buf) || S_ISDIR (st_buf.st_mode))\n\nShouldn't this be\n\tif (stat(gp, &st_buf) || !S_ISREG(st_buf.st_mode))\n\n?\n\nOtherwise, you might end up opening a FIFO, a socket or a\nblock/char special, which is obviously undesired.\n\n-- \nAndreas Ericsson andreas.ericsson@op5.se\nOP5 AB www.op5.se\nTel: +46 8-230225 Fax: +46 8-230231\n"},{"id":"68361","messageId":"20080211111741.2269f89b@pc09.procura.nl","threadId":"11969","inReplyTo":"47B01CD4.4040004@op5.se","subject":"Re: read_branches_file ()","fromName":"H.Merijn Brand","fromEmail":"h.m.brand@xs4all.nl","sentAt":"2008-02-11T10:17:41Z","receivedAt":"2008-02-11T10:17:41Z","isPatch":false,"sender":{"key":"h.m.brand@xs4all.nl","avatar":"https://gravatar.com/avatar/5b8f83ee35c427a646cbea3b104346e00ab3663b99bbf435cddeb75cd4b3857b?d=mp&s=160"},"body":"On Mon, 11 Feb 2008 11:00:52 +0100, Andreas Ericsson wrote:\n\n> H.Merijn Brand wrote:\n> > - if (!f)\n> > + if (stat (gp, &st_buf) || S_ISDIR (st_buf.st_mode))\n> \n> Shouldn't this be\n> \tif (stat(gp, &st_buf) || !S_ISREG(st_buf.st_mode))\n\nYes, see several follow-ups in this thread.\n\n> ?\n> \n> Otherwise, you might end up opening a FIFO, a socket or a\n> block/char special, which is obviously undesired.\n\nAt the moment of the patch I was unsure if these were supported :)\nAt least FIFO's and character devices are less error-prone than DIR's\n\n-- \nH.Merijn Brand Amsterdam Perl Mongers (http://amsterdam.pm.org/)\nusing & porting perl 5.6.2, 5.8.x, 5.10.x on HP-UX 10.20, 11.00, 11.11,\n& 11.23, SuSE 10.1 & 10.2, AIX 5.2, and Cygwin. http://qa.perl.org\nhttp://mirrors.develooper.com/hpux/ http://www.test-smoke.org\n http://www.goldmark.org/jeff/stupid-disclaimers/\n"}]}