svn commit: r206043 - in head/usr.sbin/pkg_install: add delete lib version

[Garrett Cooper]

Hi Florent and Robert,

On Thu, Apr 1, 2010 at 7:27 AM, Florent Thoumie <flz at freebsd.org> wrote:
> Author: flz
> Date: Thu Apr  1 14:27:29 2010
> New Revision: 206043
> URL: http://svn.freebsd.org/changeset/base/206043
>
> Log:
>  Various fixes.
>
>  - Replace hardcoded INDEX version. [1]
>  - Fix a buffer overlap. [2]
>  - Remove empty package when fetching fails and -K is used. [3]
>  - Remove useless chmod2() after mkdtemp(3). [4]
>  - Replace mkdir(1) call with mkdir(2). [5]
>  - Get rid of some vsystem() calls.
>  - Switch from lstat(2) to open(2) in fexists().
>  - Try rename(2) in move_file() first.
>  - Bump PKG_INSTALL_VERSION to 20100401.
>
>  PR:           bin/145101 [1], bin/139492 [2], bin/144919 [3]
>                bin/144920 [4], bin/144921 [5]
>  Submitted by: gcooper [1,2,3,4,5]
>
> Modified:
>  head/usr.sbin/pkg_install/add/futil.c
>  head/usr.sbin/pkg_install/add/perform.c
>  head/usr.sbin/pkg_install/delete/perform.c
>  head/usr.sbin/pkg_install/lib/file.c
>  head/usr.sbin/pkg_install/lib/lib.h
>  head/usr.sbin/pkg_install/lib/match.c
>  head/usr.sbin/pkg_install/lib/pen.c
>  head/usr.sbin/pkg_install/lib/url.c
>  head/usr.sbin/pkg_install/version/perform.c

[...]

> Modified: head/usr.sbin/pkg_install/add/perform.c
> ==============================================================================
> --- head/usr.sbin/pkg_install/add/perform.c     Thu Apr  1 13:27:27 2010        (r206042)
> +++ head/usr.sbin/pkg_install/add/perform.c     Thu Apr  1 14:27:29 2010        (r206043)
> @@ -78,6 +78,7 @@ pkg_do(char *pkg)
>     char pre_arg[FILENAME_MAX], post_arg[FILENAME_MAX];
>     char *conflict[2];
>     char **matched;
> +    int fd;

[...]

>        /* Make sure pkg_info can read the entry */
> -       vsystem("/bin/chmod a+rx %s", LogDir);
> +       fd = open(LogDir, O_RDWR);
> +       fstat(fd, &sb);
> +       fchmod(fd, sb.st_mode | S_IRALL | S_IXALL);     /* be sure, chmod a+rx */
> +       close(fd);

Yipes... we really should be checking to make sure that fchmod, fstat,
and open pass. This would look really bad if not. I would also argue
that all of the files that aren't properly chmod'ed, etc are in fact
the packager's fault and should be fixed by adding appropriate scripts
because mtree should catch this stuff if it's setup appropriately.

[...]

> Modified: head/usr.sbin/pkg_install/lib/file.c
> ==============================================================================
> --- head/usr.sbin/pkg_install/lib/file.c        Thu Apr  1 13:27:27 2010        (r206042)
> +++ head/usr.sbin/pkg_install/lib/file.c        Thu Apr  1 14:27:29 2010        (r206043)
> @@ -31,10 +31,13 @@ __FBSDID("$FreeBSD$");
>  Boolean
>  fexists(const char *fname)
>  {
> -    struct stat dummy;
> -    if (!lstat(fname, &dummy))
> -       return TRUE;
> -    return FALSE;
> +    int fd;
> +
> +    if ((fd = open(fname, O_RDONLY)) == -1)
> +       return FALSE;
> +
> +    close(fd);
> +    return TRUE;
>  }

    I was leery of this change because fexists is used widely across
pkg_install for purposes other than just determining whether or not a
file existed, in particular it's determining whether or not the
end-file exists. The original submitter for the PR didn't actually
test this point thoroughly so I need to go and write some tests to
ensure that the code isn't actually regressing on accident :/.

>  /* Quick check to see if something is a directory or symlink to a directory */
> @@ -279,17 +282,23 @@ copy_file(const char *dir, const char *f
>  }
>
>  void
> -move_file(const char *dir, const char *fname, const char *to)
> +move_file(const char *dir, const char *fname, const char *tdir)
>  {
> -    char cmd[FILENAME_MAX];
> +    char from[FILENAME_MAX];
> +    char to[FILENAME_MAX];
>
>     if (fname[0] == '/')
> -       snprintf(cmd, FILENAME_MAX, "/bin/mv %s %s", fname, to);
> +       strncpy(from, fname, FILENAME_MAX);
>     else
> -       snprintf(cmd, FILENAME_MAX, "/bin/mv %s/%s %s", dir, fname, to);
> -    if (vsystem(cmd)) {
> -       cleanup(0);
> -       errx(2, "%s: could not perform '%s'", __func__, cmd);
> +       snprintf(from, FILENAME_MAX, "%s/%s", dir, fname);
> +
> +    snprintf(to, FILENAME_MAX, "%s/%s", tdir, fname);
> +
> +    if (rename(from, to) == -1) {
> +        if (vsystem("/bin/mv %s %s", from, to)) {
> +           cleanup(0);
> +           errx(2, "%s: could not move '%s' to '%s'", __func__, from, to);
> +       }
>     }
>  }

    1. FILENAME_MAX could be less than PATH_MAX, and actually is on
the BSDs (256 vs 1024). PATH_MAX allows for duplicate slashes and all
sorts of whacky path crud and probably should be used more liberally
in the pkg_install code. This however isn't always true in the NetBSD
case because they're aiming for portability of pkg_install, however
PATH_MAX is always guaranteed to be at least as large as FILENAME_MAX.
    2. Does rename(2) copy [MAC] and other attributes properly? It
appears to do the right thing, but I could be misreading the code...
Thanks for the commits BTW :)!
-Garrett