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);
+ }
}
/*
