From owner-p4-projects@FreeBSD.ORG Wed Jun 2 09:52:33 2010 Return-Path: Delivered-To: p4-projects@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 32767) id 682D81065678; Wed, 2 Jun 2010 09:52:33 +0000 (UTC) Delivered-To: perforce@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 2C51D1065677 for ; Wed, 2 Jun 2010 09:52:33 +0000 (UTC) (envelope-from gcooper@FreeBSD.org) Received: from repoman.freebsd.org (unknown [IPv6:2001:4f8:fff6::29]) by mx1.freebsd.org (Postfix) with ESMTP id 18C1A8FC19 for ; Wed, 2 Jun 2010 09:52:33 +0000 (UTC) Received: from repoman.freebsd.org (localhost [127.0.0.1]) by repoman.freebsd.org (8.14.3/8.14.3) with ESMTP id o529qX1D049002 for ; Wed, 2 Jun 2010 09:52:33 GMT (envelope-from gcooper@FreeBSD.org) Received: (from perforce@localhost) by repoman.freebsd.org (8.14.3/8.14.3/Submit) id o529qXB9049000 for perforce@freebsd.org; Wed, 2 Jun 2010 09:52:33 GMT (envelope-from gcooper@FreeBSD.org) Date: Wed, 2 Jun 2010 09:52:33 GMT Message-Id: <201006020952.o529qXB9049000@repoman.freebsd.org> X-Authentication-Warning: repoman.freebsd.org: perforce set sender to gcooper@FreeBSD.org using -f From: Garrett Cooper To: Perforce Change Reviews Precedence: bulk Cc: Subject: PERFORCE change 179079 for review X-BeenThere: p4-projects@freebsd.org X-Mailman-Version: 2.1.5 List-Id: p4 projects tree changes List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 02 Jun 2010 09:52:33 -0000 http://p4web.freebsd.org/@@179079?ac=10 Change 179079 by gcooper@gcooper-bayonetta on 2010/06/02 09:51:53 Improve the overall flow of pkg_create as follows: 1. Only check for whether or not prefix is NULL once. 2. Add the prefix @cwd once in the application's lifetime instead of in two separate instances. 3. Don't add the metadata files to the plist. Why? Because a) they don't end up in any of the plists (installed from ports at least), and b) it causes functional issues and introduces unneeded complexity with the overall function of the package because one needs to keep track of what files are the metadata files, where they need to go (it's fluid because of the fact that one can specify $PKG_DBDIR). 4. Consolidate all of the make_dist macros at the top of the function. 5. Fix some style in perform.c's function headers and prototypes. 6. Add functionality to detect and properly install a file into if the path is absolute. 7. Make the token strcpy a strlcpy just to be safe (and to be sure that a NUL is tacked on the end). 8. Fix a bad [double-]free(3) call. This is untested code, but will be vetted soon. Affected files ... .. //depot/projects/soc2007/gcooper-pkg_install-enhancements-simplified/usr.sbin/pkg_install/create/perform.c#32 edit Differences ... ==== //depot/projects/soc2007/gcooper-pkg_install-enhancements-simplified/usr.sbin/pkg_install/create/perform.c#32 (text+ko) ==== @@ -21,8 +21,8 @@ #include __FBSDID("$FreeBSD$"); -/* XXX (gcooper): needs to come before sys/stat.h for stat(2). */ -#include + +#include /* needs to come before sys/stat.h for stat(2). */ /* Read comment below in add_file. */ #ifdef BROKEN_MMAP #include @@ -234,9 +234,9 @@ } /* Prefix should add an @cwd to the packing list */ - if (Prefix) - if (add_plist_top(&plist, PLIST_CWD, Prefix) == -1) - err(EXIT_FAILURE, "%s: add_plist_top failed", __func__); + if (add_plist_top(&plist, PLIST_CWD, (Prefix == NULL ? "." : Prefix)) == + -1) + err(EXIT_FAILURE, "%s: add_plist_top failed", __func__); /* Add the origin if asked, at the top */ if (Origin) @@ -252,7 +252,7 @@ err(EXIT_FAILURE, "%s: add_plist_top failed", __func__); if (asprintf(&cp, "PKG_FORMAT_REVISION:%d.%d", PLIST_FMT_VER_MAJOR, - PLIST_FMT_VER_MINOR) == -1) { + PLIST_FMT_VER_MINOR) == -1) { errx(2, "%s: asprintf() failed", __func__); } if (add_plist_top(&plist, PLIST_COMMENT, cp) == -1) @@ -263,7 +263,7 @@ * We're just here for to dump out a revised plist for the FreeBSD ports * hack. It's not a real create in progress. */ - if (PlistOnly) { + if (PlistOnly == TRUE) { check_list(home, &plist); exit(write_plist(&plist, stdout) == 0 ? 0 : 1); } @@ -281,82 +281,12 @@ /* copy_plist(home, &plist); */ /* mark_plist(&plist); */ - /* Now put the release specific items in */ - if (Prefix == NULL) - if (add_plist(&plist, PLIST_CWD, ".") == -1) - err(EXIT_FAILURE, "%s: add_plist failed", __func__); - - if (write_file(COMMENT_FNAME, Comment) == 0) { - if (add_plist(&plist, PLIST_IGNORE, NULL) == -1) - err(EXIT_FAILURE, "%s: add_plist failed", __func__); - if (add_plist(&plist, PLIST_FILE, COMMENT_FNAME) == -1) - err(EXIT_FAILURE, "%s: add_plist failed", __func__); - add_cksum(&plist, plist.tail, COMMENT_FNAME); - } else + /* Write out required files */ + if (write_file(COMMENT_FNAME, Comment) == -1) err(EXIT_FAILURE, "failed to write comment file"); - if (write_file(DESC_FNAME, Desc) == 0) { - if (add_plist(&plist, PLIST_IGNORE, NULL) == -1) - err(EXIT_FAILURE, "%s: add_plist failed", __func__); - if (add_plist(&plist, PLIST_FILE, DESC_FNAME) == -1) - err(EXIT_FAILURE, "%s: add_plist failed", __func__); - add_cksum(&plist, plist.tail, DESC_FNAME); - } else + if (write_file(DESC_FNAME, Desc) == -1) err(EXIT_FAILURE, "failed to write description file"); - if (Install != NULL) { - if (add_plist(&plist, PLIST_IGNORE, NULL) == -1) - err(EXIT_FAILURE, "%s: add_plist failed", __func__); - if (add_plist(&plist, PLIST_FILE, INSTALL_FNAME) == -1) - err(EXIT_FAILURE, "%s: add_plist failed", __func__); - add_cksum(&plist, plist.tail, INSTALL_FNAME); - } - if (PostInstall != NULL) { - if (add_plist(&plist, PLIST_IGNORE, NULL) == -1) - err(EXIT_FAILURE, "%s: add_plist failed", __func__); - if (add_plist(&plist, PLIST_FILE, POST_INSTALL_FNAME) == -1) - err(EXIT_FAILURE, "%s: add_plist failed", __func__); - add_cksum(&plist, plist.tail, POST_INSTALL_FNAME); - } - if (DeInstall != NULL) { - if (add_plist(&plist, PLIST_IGNORE, NULL) == -1) - err(EXIT_FAILURE, "%s: add_plist failed", __func__); - if (add_plist(&plist, PLIST_FILE, DEINSTALL_FNAME) == -1) - err(EXIT_FAILURE, "%s: add_plist failed", __func__); - add_cksum(&plist, plist.tail, DEINSTALL_FNAME); - } - if (PostDeInstall != NULL) { - if (add_plist(&plist, PLIST_IGNORE, NULL) == -1) - err(EXIT_FAILURE, "%s: add_plist failed", __func__); - if (add_plist(&plist, PLIST_FILE, POST_DEINSTALL_FNAME) == -1) - err(EXIT_FAILURE, "%s: add_plist failed", __func__); - add_cksum(&plist, plist.tail, POST_DEINSTALL_FNAME); - } - if (Require != NULL) { - if (add_plist(&plist, PLIST_IGNORE, NULL) == -1) - err(EXIT_FAILURE, "%s: add_plist failed", __func__); - if (add_plist(&plist, PLIST_FILE, REQUIRE_FNAME) == -1) - err(EXIT_FAILURE, "%s: add_plist failed", __func__); - add_cksum(&plist, plist.tail, REQUIRE_FNAME); - } - if (Display != NULL) { - if (add_plist(&plist, PLIST_IGNORE, NULL) == -1) - err(EXIT_FAILURE, "%s: add_plist failed", __func__); - if (add_plist(&plist, PLIST_FILE, DISPLAY_FNAME) == -1) - err(EXIT_FAILURE, "%s: add_plist failed", __func__); - add_cksum(&plist, plist.tail, DISPLAY_FNAME); - if (add_plist(&plist, PLIST_DISPLAY, DISPLAY_FNAME) == -1) - err(EXIT_FAILURE, "%s: add_plist failed", __func__); - } - if (Mtree != NULL) { - if (add_plist(&plist, PLIST_IGNORE, NULL) == -1) - err(EXIT_FAILURE, "%s: add_plist failed", __func__); - if (add_plist(&plist, PLIST_FILE, MTREE_FNAME) == -1) - err(EXIT_FAILURE, "%s: add_plist failed", __func__); - add_cksum(&plist, plist.tail, MTREE_FNAME); - if (add_plist(&plist, PLIST_MTREE, MTREE_FNAME) == -1) - err(EXIT_FAILURE, "%s: add_plist failed", __func__); - } - /* Finally, write out the packing list */ fp = fopen(CONTENTS_FNAME, "w"); if (!fp) @@ -378,7 +308,7 @@ return TRUE; /* Success */ } -static const char* +static const char* add_file(struct archive *archive, const char *srcfile, const char *destfile, const int archive_entry_open_flags) { @@ -469,16 +399,37 @@ } static void -make_dist(const char *homedir, const char *pkg, const char *suff, Package *plist) +make_dist(const char *homedir, const char *pkg, const char *suff, + Package *plist) { -#define ADD_FILE(SRCFILE, DESTFILE, OPEN_FLAGS) \ - if (error == NULL) { \ - error = add_file(archive, SRCFILE, DESTFILE, \ +#define ADD_FILE(SRCFILE, DESTFILE, OPEN_FLAGS) do { \ + if (error == NULL) { \ + error = add_file(archive, SRCFILE, \ + DESTFILE, OPEN_FLAGS); \ + } \ + } while (0) + +#define ADD_METADATA_FILE(METADATA_FILE, OPEN_FLAGS) do { \ + ADD_FILE(METADATA_FILE, METADATA_FILE, \ OPEN_FLAGS); \ - } + } while (0) + +#define ADD_TRAILING_SLASH(v, v_len) \ + do { \ + v_len = strlen(v); \ + if (v[v_len-1] != '/') { \ + if (v_len >= PATH_MAX) \ + error = strerror(ENAMETOOLONG); \ + else { \ + v[v_len] = '/'; \ + v[v_len+1] = '\0'; \ + } \ + } \ + } while (0) PackingList p; + char tball[PATH_MAX]; struct archive *archive = NULL; struct lafe_matching *match_patterns = NULL; char *destbase = NULL; @@ -486,15 +437,16 @@ char *prefix = NULL; char *srcbase = NULL; char *srcfile = NULL; - char tball[PATH_MAX]; const char *cname = NULL; const char *error = NULL; + int archive_fd = -1; int archive_open_flags; int archive_entry_open_flags; int archive_metadata_open_flags; - int destbase_len, srcbase_len; + int destbase_len; + int srcbase_len; if (*pkg == '/') snprintf(tball, sizeof(tball), "%s.%s", pkg, suff); @@ -577,11 +529,102 @@ } - if (error == NULL && - archive_write_open_fd(archive, archive_fd) != ARCHIVE_OK) - error = archive_error_string(archive); + if (error == NULL) { + + if (archive_write_open_fd(archive, archive_fd) != ARCHIVE_OK) + error = archive_error_string(archive); + else if ((p = find_plist(plist, PLIST_CWD)) != NULL && + p->name == NULL) { + + warnx("malformed plist (first @cwd is NULL)"); + error = strerror(EINVAL); + + } else { /* first @cwd -- wewt! */ + + if (strlen(p->name) > PATH_MAX) + error = strerror(ENAMETOOLONG); + else { + + /* + * We're relative to the starting prefix; + * dump the files here. + * + * This usecase only makes sense when + * installing packages directly to '/'. + * + * All metadata files will be installed into + * /var/db/ by pkg_add . + */ + if (strlen(p->name) == 1 && + p->name[0] == '.' && + getcwd(prefix, sizeof(prefix)) == NULL) + error = strerror(errno); + else if (strlcpy(prefix, p->name, + sizeof(prefix) >= sizeof(prefix))) + error = strerror(ENAMETOOLONG); + + } + + if (error == NULL) { + + /* Tack BaseDir on the front if defined. */ + if (BaseDir != NULL) { + if (strlcpy(srcbase, BaseDir, + PATH_MAX) > PATH_MAX) + error = strerror(ENAMETOOLONG); + } else + srcbase[0] = '\0'; + + } + + } + + /* + * Add all metadata files; these don't go in the plist for + * the sake of simplicity. + */ + ADD_METADATA_FILE(COMMENT_FNAME, + archive_entry_open_flags); + + ADD_METADATA_FILE(DESC_FNAME, + archive_entry_open_flags); + + if (Install != NULL) { + ADD_METADATA_FILE(INSTALL_FNAME, + archive_entry_open_flags); + } + if (PostInstall != NULL) { + ADD_METADATA_FILE(POST_INSTALL_FNAME, + archive_entry_open_flags); + } + if (DeInstall != NULL) { + ADD_METADATA_FILE(DEINSTALL_FNAME, + archive_entry_open_flags); + } + if (PostDeInstall != NULL) { + ADD_METADATA_FILE(POST_DEINSTALL_FNAME, + archive_entry_open_flags); + } + if (Require != NULL) { + ADD_METADATA_FILE(REQUIRE_FNAME, + archive_entry_open_flags); + } + if (Display != NULL) { + ADD_METADATA_FILE(DISPLAY_FNAME, + archive_entry_open_flags); + } + if (Mtree != NULL) { + ADD_METADATA_FILE(MTREE_FNAME, + archive_entry_open_flags); + } + + } + + /* Let's iterate from the first @cwd. */ + if (p == NULL) + p = plist->head; - for (p = plist->head; error == NULL && p != NULL; p = p->next) + for ( ; error == NULL && p != NULL; p = p->next) switch(p->type) { case PLIST_FILE: @@ -593,10 +636,12 @@ destfile[0] = srcfile[0] = '\0'; /* - * File is based off the current working directory if - * NULL. + * Don't prefix with destbase if: + * 1. File is absolute (starts with '/'). + * 2. destbase is NULL (file is based off the current + * working directory). */ - if (destbase != NULL) + if (*p->name != '/' && destbase != NULL) if (strlcat(destfile, destbase, PATH_MAX) > PATH_MAX) error = strerror(ENAMETOOLONG); @@ -629,20 +674,8 @@ /* Reset to / */ if (p->name == NULL) { - /* - * Broken plist; prefix must be defined before - * calling `@cwd' with variable following - * whitespace. - */ - assert(prefix != NULL); - /* - * NOTE (gcooper): strcpy is safe here so long - * as the buffers are of equal size, and also - * because the value has been sanitized below - * and because of the assert above. - */ - strcpy(destbase, prefix); + strlcpy(destbase, prefix, sizeof(destbase)); /* Reset srcbase */ /* Tack BaseDir on the front if defined. */ @@ -662,32 +695,6 @@ */ else { - /* First @cwd -- wewt! */ - if (prefix == NULL) { - - if (strlen(p->name) > PATH_MAX) - error = strerror(ENAMETOOLONG); - else { - - prefix = p->name; - - /* - * Tack BaseDir on the front if - * defined and this is the first - * run. - */ - if (BaseDir != NULL) { - if (strlcpy(srcbase, - BaseDir, PATH_MAX) > - PATH_MAX) - error = strerror(ENAMETOOLONG); - } else - srcbase[0] = '\0'; - - } - - } - if (strlcat(destbase, p->name, PATH_MAX) > PATH_MAX) error = strerror(ENAMETOOLONG); @@ -705,19 +712,6 @@ */ if (error == NULL) { -#define ADD_TRAILING_SLASH(v, v_len) \ - do { \ - v_len = strlen(v); \ - if (v[v_len-1] != '/') { \ - if (v_len >= PATH_MAX) \ - error = strerror(ENAMETOOLONG); \ - else { \ - v[v_len] = '/'; \ - v[v_len+1] = '\0'; \ - } \ - } \ - } while (0) - ADD_TRAILING_SLASH(destbase, destbase_len); ADD_TRAILING_SLASH(srcbase, srcbase_len); @@ -757,21 +751,22 @@ warnx("%s: unable to create the package '%s': %s", __func__, tball, error); if (0 <= archive_fd) { - close(archive_fd); + (void) close(archive_fd); if (error != NULL && unlink(tball) == -1) warn("%s: failed to remove incomplete package - '%s'", __func__, tball); } + free(destbase); free(destfile); + free(prefix); free(srcbase); free(srcfile); - free(destfile); } static void -sanity_check() +sanity_check(void) { if (!Comment) errx(2, "%s: required package comment string is missing (-c comment)", @@ -817,7 +812,7 @@ return rval; } rc = read_plist(&plist, fd); - close(fd); + (void) close(fd); if (rc == 0) { rval = TRUE;