install(1) broken

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

install(1) broken

Marc Espie-2
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

Reply | Threaded
Open this post in threaded view
|

Re: install(1) broken

Ingo Schwarze
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

Reply | Threaded
Open this post in threaded view
|

Re: install(1) broken

Ted Unangst-6
In reply to this post by Marc Espie-2
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 -0000 1.68
+++ xinstall.c 9 Feb 2019 22:21:03 -0000
@@ -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);
+ }
  }
 
  /*

Reply | Threaded
Open this post in threaded view
|

Re: install(1) broken

Marc Espie-2
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.

Reply | Threaded
Open this post in threaded view
|

Re: install(1) broken

Marc Espie-2
In reply to this post by Ted Unangst-6
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.c 8 Feb 2019 12:53:44 -0000 1.68
> +++ xinstall.c 9 Feb 2019 22:21:03 -0000
> @@ -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...

Reply | Threaded
Open this post in threaded view
|

Re: install(1) broken

Marc Espie-2
In reply to this post by Ted Unangst-6
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 -0000 1.68
+++ xinstall.c 10 Feb 2019 14:53:49 -0000
@@ -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 *)&from_sb, 0, sizeof(from_sb));
  (void)memset((void *)&to_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)

Reply | Threaded
Open this post in threaded view
|

Re: install(1) broken

Ted Unangst-6
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.

Reply | Threaded
Open this post in threaded view
|

Re: install(1) broken

Marc Espie-2
In reply to this post by Marc Espie-2
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 -0000 1.68
+++ xinstall.c 10 Feb 2019 14:53:49 -0000
@@ -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 *)&from_sb, 0, sizeof(from_sb));
  (void)memset((void *)&to_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)