Re: install(1) broken
On Sun, Feb 10, 2019 at 03:55:41PM +0100, Marc Espie wrote: > Something similar to this perhaps ? > Not fully tested yet, but it should avoid the race of trying to > unlink tempfile several times, and also fix the file name in error messages. That's now been tested (gone thru a build + xbuild and bulk in progress that went thru go-bootstrap and go without issues). Okay ? This needs re-applying Ingo's patch first, of course, which I can either do or we coordinate with Ingo. Index: xinstall.c === RCS file: /cvs/src/usr.bin/xinstall/xinstall.c,v retrieving revision 1.68 diff -u -p -r1.68 xinstall.c --- xinstall.c 8 Feb 2019 12:53:44 - 1.68 +++ xinstall.c 10 Feb 2019 14:53:49 - @@ -222,6 +222,7 @@ install(char *from_name, char *to_name, struct timespec ts[2]; int devnull, from_fd, to_fd, serrno, files_match = 0; char *p; + char *target_name = tempfile; (void)memset((void *)_sb, 0, sizeof(from_sb)); (void)memset((void *)_sb, 0, sizeof(to_sb)); @@ -311,10 +312,14 @@ install(char *from_name, char *to_name, } else { files_match = 1; (void)unlink(tempfile); + target_name = to_name; + (void)close(temp_fd); } } - (void)close(to_fd); - to_fd = temp_fd; + if (!files_match) { + (void)close(to_fd); + to_fd = temp_fd; + } } /* @@ -333,13 +338,15 @@ install(char *from_name, char *to_name, if ((gid != (gid_t)-1 || uid != (uid_t)-1) && fchown(to_fd, uid, gid)) { serrno = errno; - (void)unlink(tempfile); - errx(1, "%s: chown/chgrp: %s", tempfile, strerror(serrno)); + if (target_name == tempfile) + (void)unlink(tempfile); + errx(1, "%s: chown/chgrp: %s", target_name, strerror(serrno)); } if (fchmod(to_fd, mode)) { serrno = errno; - (void)unlink(tempfile); - errx(1, "%s: chmod: %s", tempfile, strerror(serrno)); + if (target_name == tempfile) + (void)unlink(tempfile); + errx(1, "%s: chmod: %s", target_name, strerror(serrno)); } /* @@ -349,7 +356,7 @@ install(char *from_name, char *to_name, if (fchflags(to_fd, flags & SETFLAGS ? fset : from_sb.st_flags & ~UF_NODUMP)) { if (errno != EOPNOTSUPP || (from_sb.st_flags & ~UF_NODUMP) != 0) - warnx("%s: chflags: %s", tempfile, strerror(errno)); + warnx("%s: chflags: %s", target_name, strerror(errno)); } if (flags & USEFSYNC)
Re: install(1) broken
Marc Espie wrote: > Something similar to this perhaps ? > Not fully tested yet, but it should avoid the race of trying to > unlink tempfile several times, and also fix the file name in error messages. That's probably better.
Re: install(1) broken
Something similar to this perhaps ? Not fully tested yet, but it should avoid the race of trying to unlink tempfile several times, and also fix the file name in error messages. Index: xinstall.c === RCS file: /cvs/src/usr.bin/xinstall/xinstall.c,v retrieving revision 1.68 diff -u -p -r1.68 xinstall.c --- xinstall.c 8 Feb 2019 12:53:44 - 1.68 +++ xinstall.c 10 Feb 2019 14:53:49 - @@ -222,6 +222,7 @@ install(char *from_name, char *to_name, struct timespec ts[2]; int devnull, from_fd, to_fd, serrno, files_match = 0; char *p; + char *target_name = tempfile; (void)memset((void *)_sb, 0, sizeof(from_sb)); (void)memset((void *)_sb, 0, sizeof(to_sb)); @@ -311,10 +312,14 @@ install(char *from_name, char *to_name, } else { files_match = 1; (void)unlink(tempfile); + target_name = to_name; + (void)close(temp_fd); } } - (void)close(to_fd); - to_fd = temp_fd; + if (!files_match) { + (void)close(to_fd); + to_fd = temp_fd; + } } /* @@ -333,13 +338,15 @@ install(char *from_name, char *to_name, if ((gid != (gid_t)-1 || uid != (uid_t)-1) && fchown(to_fd, uid, gid)) { serrno = errno; - (void)unlink(tempfile); - errx(1, "%s: chown/chgrp: %s", tempfile, strerror(serrno)); + if (target_name == tempfile) + (void)unlink(tempfile); + errx(1, "%s: chown/chgrp: %s", target_name, strerror(serrno)); } if (fchmod(to_fd, mode)) { serrno = errno; - (void)unlink(tempfile); - errx(1, "%s: chmod: %s", tempfile, strerror(serrno)); + if (target_name == tempfile) + (void)unlink(tempfile); + errx(1, "%s: chmod: %s", target_name, strerror(serrno)); } /* @@ -349,7 +356,7 @@ install(char *from_name, char *to_name, if (fchflags(to_fd, flags & SETFLAGS ? fset : from_sb.st_flags & ~UF_NODUMP)) { if (errno != EOPNOTSUPP || (from_sb.st_flags & ~UF_NODUMP) != 0) - warnx("%s: chflags: %s", tempfile, strerror(errno)); + warnx("%s: chflags: %s", target_name, strerror(errno)); } if (flags & USEFSYNC)
Re: install(1) broken
On Sat, Feb 09, 2019 at 05:23:09PM -0500, Ted Unangst wrote: > Marc Espie wrote: > > hey, your commit to install(1) broke something. > > > > Specifically lang/go-boostrap now produces a broken package which can't > > be used to build go. > > > > All the go/bootstrap/pkg/tool/openbsd_amd64/* > > have lost their x bit > > > > Relevant fake install information, it definitely looks like the last line > > is now a no-op. > > > # These get installed via `find' however we need them to be executable > > /pobj/go-bootstrap-1.4.20171003/bin/install -d -m 755 > > /pobj/go-bootstrap-1.4.20171003/fake-amd64/usr/local/go/bootstrap/pkg/tool//openbsd_amd64 > > /pobj/go-bootstrap-1.4.20171003/bin/install -c -m 755 -p > > /pobj/go-bootstrap-1.4.20171003/go/pkg/tool//openbsd_amd64/* > > /pobj/go-bootstrap-1.4.20171003/fake-amd64/usr/local/go/bootstrap/pkg/tool//openbsd_amd64 > > Yes. This is a weird way to invoke chmod, but that's what it wants. > > I think this was a long standing bug in install? If the files match, we need > to continue on with to_fd == existing file so that metadata updates work. > > > Index: xinstall.c > === > RCS file: /cvs/src/usr.bin/xinstall/xinstall.c,v > retrieving revision 1.68 > diff -u -p -r1.68 xinstall.c > --- xinstall.c8 Feb 2019 12:53:44 - 1.68 > +++ xinstall.c9 Feb 2019 22:21:03 - > @@ -313,8 +313,12 @@ install(char *from_name, char *to_name, > (void)unlink(tempfile); > } > } > - (void)close(to_fd); > - to_fd = temp_fd; > + if (!files_match) { > + (void)close(to_fd); > + to_fd = temp_fd; > + } else { > + close(temp_fd); > + } > } > > /* I'm afraid this needs to be slightly more complex, probably to put the remaining code in its own function, or something. Specifically, the error paths for the fchown and fchmod will be all bogus in that case, referring to a tempfile that's already been removed, and is not even the target...
Re: install(1) broken
On Sat, Feb 09, 2019 at 05:23:09PM -0500, Ted Unangst wrote: > Marc Espie wrote: > > hey, your commit to install(1) broke something. > > > > Specifically lang/go-boostrap now produces a broken package which can't > > be used to build go. > > > > All the go/bootstrap/pkg/tool/openbsd_amd64/* > > have lost their x bit > > > > Relevant fake install information, it definitely looks like the last line > > is now a no-op. > > > # These get installed via `find' however we need them to be executable > > /pobj/go-bootstrap-1.4.20171003/bin/install -d -m 755 > > /pobj/go-bootstrap-1.4.20171003/fake-amd64/usr/local/go/bootstrap/pkg/tool//openbsd_amd64 > > /pobj/go-bootstrap-1.4.20171003/bin/install -c -m 755 -p > > /pobj/go-bootstrap-1.4.20171003/go/pkg/tool//openbsd_amd64/* > > /pobj/go-bootstrap-1.4.20171003/fake-amd64/usr/local/go/bootstrap/pkg/tool//openbsd_amd64 > > Yes. This is a weird way to invoke chmod, but that's what it wants. This actually makes some kind of sense in a broader context. Specifically, install_flags are a somewhat standard way to enforce ownership/permissions on a file, whether while copying it from one place to another... or after moving it. It's way simpler to bundle everything into a single variable than having several separate variables to do things.
Re: install(1) broken
Marc Espie wrote: > hey, your commit to install(1) broke something. > > Specifically lang/go-boostrap now produces a broken package which can't > be used to build go. > > All the go/bootstrap/pkg/tool/openbsd_amd64/* > have lost their x bit > > Relevant fake install information, it definitely looks like the last line > is now a no-op. > # These get installed via `find' however we need them to be executable > /pobj/go-bootstrap-1.4.20171003/bin/install -d -m 755 > /pobj/go-bootstrap-1.4.20171003/fake-amd64/usr/local/go/bootstrap/pkg/tool//openbsd_amd64 > /pobj/go-bootstrap-1.4.20171003/bin/install -c -m 755 -p > /pobj/go-bootstrap-1.4.20171003/go/pkg/tool//openbsd_amd64/* > /pobj/go-bootstrap-1.4.20171003/fake-amd64/usr/local/go/bootstrap/pkg/tool//openbsd_amd64 Yes. This is a weird way to invoke chmod, but that's what it wants. I think this was a long standing bug in install? If the files match, we need to continue on with to_fd == existing file so that metadata updates work. Index: xinstall.c === RCS file: /cvs/src/usr.bin/xinstall/xinstall.c,v retrieving revision 1.68 diff -u -p -r1.68 xinstall.c --- xinstall.c 8 Feb 2019 12:53:44 - 1.68 +++ xinstall.c 9 Feb 2019 22:21:03 - @@ -313,8 +313,12 @@ install(char *from_name, char *to_name, (void)unlink(tempfile); } } - (void)close(to_fd); - to_fd = temp_fd; + if (!files_match) { + (void)close(to_fd); + to_fd = temp_fd; + } else { + close(temp_fd); + } } /*
Re: install(1) broken
Hi Marc, Marc Espie wrote on Sat, Feb 09, 2019 at 10:03:20PM +0100: > hey, your commit to install(1) broke something. > > Specifically lang/go-boostrap now produces a broken package which can't > be used to build go. > > All the go/bootstrap/pkg/tool/openbsd_amd64/* > have lost their x bit > > Relevant fake install information, it definitely looks like the last line > is now a no-op. [...] > install -c -m 755 -p [...] Sorry for the disruption. It looks like the bug was not caused, but merely exposed by my commit. I reverted my commit anyway such than we can first fix the bug without a hurry, then reapply my change. I did not revert the manual page change to minimize churn, expecting that the change will eventually be put back. A preliminary analysis indicates that 1. install -m 755 foo bar always applies 755 to bar 2. install -S-m 755 foo bar always applies 755 to bar 3. install -S -p -m 755 foo bar only applies 755 if the file content changes Item 3 looks like a bug to me. Similar effects may or may not apply to the owner and the flags. I will look in more detail later. Yours, Ingo
install(1) broken
hey, your commit to install(1) broke something. Specifically lang/go-boostrap now produces a broken package which can't be used to build go. All the go/bootstrap/pkg/tool/openbsd_amd64/* have lost their x bit Relevant fake install information, it definitely looks like the last line is now a no-op. >>> Running fake in lang/go-bootstrap at 1549742487 ===> lang/go-bootstrap ===> Faking installation for go-bootstrap-1.4.20171003p0 /pobj/go-bootstrap-1.4.20171003/bin/install -d -m 755 /pobj/go-bootstrap-1.4.20171003/fake-amd64/usr/local/go/bootstrap /pobj/go-bootstrap-1.4.20171003/bin/install -d -m 755 /pobj/go-bootstrap-1.4.20171003/fake-amd64/usr/local/go/bootstrap/bin /pobj/go-bootstrap-1.4.20171003/bin/install -c -m 755 -p /pobj/go-bootstrap-1.4.20171003/go/bin/go{,fmt} /pobj/go-bootstrap-1.4.20171003/fake-amd64/usr/local/go/bootstrap/bin find /pobj/go-bootstrap-1.4.20171003/go -maxdepth 1 -type f ! -name .git\* ! -name .hg\* -exec /pobj/go-bootstrap-1.4.20171003/bin/install -c -m 644 -p {} /pobj/go-bootstrap-1.4.20171003/fake-amd64/usr/local/go/bootstrap \; cd /pobj/go-bootstrap-1.4.20171003/go && find doc -type d -exec /pobj/go-bootstrap-1.4.20171003/bin/install -d -m 755 /pobj/go-bootstrap-1.4.20171003/fake-amd64/usr/local/go/bootstrap/{} \; -o -type f ! -name \*.orig -exec /pobj/go-bootstrap-1.4.20171003/bin/install -c -m 644 -p {} /pobj/go-bootstrap-1.4.20171003/fake-amd64/usr/local/go/bootstrap/{} \; cd /pobj/go-bootstrap-1.4.20171003/go && find include -type d -exec /pobj/go-bootstrap-1.4.20171003/bin/install -d -m 755 /pobj/go-bootstrap-1.4.20171003/fake-amd64/usr/local/go/bootstrap/{} \; -o -type f ! -name \*.orig -exec /pobj/go-bootstrap-1.4.20171003/bin/install -c -m 644 -p {} /pobj/go-bootstrap-1.4.20171003/fake-amd64/usr/local/go/bootstrap/{} \; cd /pobj/go-bootstrap-1.4.20171003/go && find lib -type d -exec /pobj/go-bootstrap-1.4.20171003/bin/install -d -m 755 /pobj/go-bootstrap-1.4.20171003/fake-amd64/usr/local/go/bootstrap/{} \; -o -type f ! -name \*.orig -exec /pobj/go-bootstrap-1.4.20171003/bin/install -c -m 644 -p {} /pobj/go-bootstrap-1.4.20171003/fake-amd64/usr/local/go/bootstrap/{} \; cd /pobj/go-bootstrap-1.4.20171003/go && find misc -type d -exec /pobj/go-bootstrap-1.4.20171003/bin/install -d -m 755 /pobj/go-bootstrap-1.4.20171003/fake-amd64/usr/local/go/bootstrap/{} \; -o -type f ! -name \*.orig -exec /pobj/go-bootstrap-1.4.20171003/bin/install -c -m 644 -p {} /pobj/go-bootstrap-1.4.20171003/fake-amd64/usr/local/go/bootstrap/{} \; cd /pobj/go-bootstrap-1.4.20171003/go && find src -type d -exec /pobj/go-bootstrap-1.4.20171003/bin/install -d -m 755 /pobj/go-bootstrap-1.4.20171003/fake-amd64/usr/local/go/bootstrap/{} \; -o -type f ! -name \*.orig -exec /pobj/go-bootstrap-1.4.20171003/bin/install -c -m 644 -p {} /pobj/go-bootstrap-1.4.20171003/fake-amd64/usr/local/go/bootstrap/{} \; cd /pobj/go-bootstrap-1.4.20171003/go && find pkg -type d -exec /pobj/go-bootstrap-1.4.20171003/bin/install -d -m 755 /pobj/go-bootstrap-1.4.20171003/fake-amd64/usr/local/go/bootstrap/{} \; -o -type f ! -name \*.orig -exec /pobj/go-bootstrap-1.4.20171003/bin/install -c -m 644 -p {} /pobj/go-bootstrap-1.4.20171003/fake-amd64/usr/local/go/bootstrap/{} \; # These get installed via `find' however we need them to be executable /pobj/go-bootstrap-1.4.20171003/bin/install -d -m 755 /pobj/go-bootstrap-1.4.20171003/fake-amd64/usr/local/go/bootstrap/pkg/tool//openbsd_amd64 /pobj/go-bootstrap-1.4.20171003/bin/install -c -m 755 -p /pobj/go-bootstrap-1.4.20171003/go/pkg/tool//openbsd_amd64/* /pobj/go-bootstrap-1.4.20171003/fake-amd64/usr/local/go/bootstrap/pkg/tool//openbsd_amd64