Date: Mon, 31 May 2010 00:59:54 GMT From: Garrett Cooper <gcooper@FreeBSD.org> To: Perforce Change Reviews <perforce@FreeBSD.org> Subject: PERFORCE change 178990 for review Message-ID: <201005310059.o4V0xs5q050262@repoman.freebsd.org>
next in thread | raw e-mail | index | archive | help
http://p4web.freebsd.org/@@178990?ac=10 Change 178990 by gcooper@gcooper-bayonetta on 2010/05/31 00:59:14 1. Properly check for result from new_plist_entry instead of assuming it will always pass. 2. Use for-loops instead of while-loops wherever easily acceptable. 3. Use explicit branch test values. 4. Avoid a NULL pointer deref in new_plist_entry if the malloc(3) fails. 5. delete_hierarchy: i. Unify return code mechanism. ii. Avoid some NULL pointer derefs in the nukedirs loop. iii. Don't leak memory between each time that dir is strdup(3)'ed. 5. style(9)-ize code. Affected files ... .. //depot/projects/soc2007/gcooper-pkg_install-enhancements-simplified/lib/libpkg/plist.c#8 edit Differences ... ==== //depot/projects/soc2007/gcooper-pkg_install-enhancements-simplified/lib/libpkg/plist.c#8 (text+ko) ==== @@ -38,96 +38,102 @@ void add_plist(Package *p, plist_t type, const char *arg) { - PackingList tmp; + PackingList tmp; + + tmp = new_plist_entry(); + + if (tmp != NULL) { + + tmp->name = copy_string(arg); + tmp->type = type; + + if (!p->head) + p->head = p->tail = tmp; + else { + tmp->prev = p->tail; + p->tail->next = tmp; + p->tail = tmp; + } + switch (type) { + case PLIST_NAME: + p->name = tmp->name; + break; - tmp = new_plist_entry(); - tmp->name = copy_string(arg); - tmp->type = type; + case PLIST_ORIGIN: + p->origin = tmp->name; + break; - if (!p->head) - p->head = p->tail = tmp; - else { - tmp->prev = p->tail; - p->tail->next = tmp; - p->tail = tmp; - } - switch (type) { - case PLIST_NAME: - p->name = tmp->name; - break; + default: + break; + } - case PLIST_ORIGIN: - p->origin = tmp->name; - break; + } - default: - break; - } } void add_plist_top(Package *p, plist_t type, const char *arg) { - PackingList tmp; + PackingList tmp; + + tmp = new_plist_entry(); + + if (tmp != NULL) { + + tmp->name = copy_string(arg); + tmp->type = type; + + if (!p->head) + p->head = p->tail = tmp; + else { + tmp->next = p->head; + p->head->prev = tmp; + p->head = tmp; + } - tmp = new_plist_entry(); - tmp->name = copy_string(arg); - tmp->type = type; + } - if (!p->head) - p->head = p->tail = tmp; - else { - tmp->next = p->head; - p->head->prev = tmp; - p->head = tmp; - } } /* Return the last (most recent) entry in a packing list */ PackingList last_plist(Package *p) { - return p->tail; + return (p->tail); } /* Mark all items in a packing list to prevent iteration over them */ void mark_plist(Package *pkg) { - PackingList p = pkg->head; + PackingList p; - while (p) { - p->marked = TRUE; - p = p->next; - } + for (p = pkg->head; p != NULL; p = p->next) + p->marked = TRUE; } /* Find a given item in a packing list and, if so, return it (else NULL) */ PackingList find_plist(Package *pkg, plist_t type) { - PackingList p = pkg->head; + PackingList p; - while (p) { - if (p->type == type) - return p; - p = p->next; - } - return NULL; + for (p = pkg->head; p != NULL; p = p->next) + if (p->type == type) + return (p); + return (NULL); } /* Look for a specific boolean option argument in the list */ char * find_plist_option(Package *pkg, const char *name) { - PackingList p = pkg->head; + PackingList p; - while (p) { - if (p->type == PLIST_OPTION && !strcmp(p->name, name)) - return p->name; - p = p->next; - } - return NULL; + for (p = pkg->head; p != NULL; p = p->next) + if (p->type == PLIST_OPTION && !strcmp(p->name, name)) + return (p->name); + return (NULL); } /* @@ -137,56 +143,66 @@ void delete_plist(Package *pkg, Boolean all, plist_t type, const char *name) { - PackingList p = pkg->head; + PackingList p = pkg->head; + + while (p != NULL) { + + PackingList pnext = p->next; + + if (p->type == type && (!name || !strcmp(name, p->name))) { + + free(p->name); + + if (p->prev) + p->prev->next = pnext; + else + pkg->head = pnext; + if (pnext != NULL) + pnext->prev = p->prev; + else + pkg->tail = p->prev; + free(p); + + if (all == FALSE) + return; + p = pnext; - while (p) { - PackingList pnext = p->next; + } else + p = p->next; - if (p->type == type && (!name || !strcmp(name, p->name))) { - free(p->name); - if (p->prev) - p->prev->next = pnext; - else - pkg->head = pnext; - if (pnext) - pnext->prev = p->prev; - else - pkg->tail = p->prev; - free(p); - if (!all) - return; - p = pnext; } - else - p = p->next; - } + } /* Allocate a new packing list entry */ PackingList new_plist_entry(void) { - PackingList ret; + PackingList ret; - ret = (PackingList)malloc(sizeof(struct _plist)); - bzero(ret, sizeof(struct _plist)); - return ret; + ret = (PackingList)malloc(sizeof(struct _plist)); + if (ret != NULL) + bzero(ret, sizeof(struct _plist)); + return (ret); } /* Free an entire packing list */ void free_plist(Package *pkg) { - PackingList p = pkg->head; + PackingList p; + + for (p = pkg->head; p != NULL; ) { + + PackingList p1 = p->next; + + free(p->name); + free(p); + p = p1; + } - while (p) { - PackingList p1 = p->next; + pkg->head = pkg->tail = NULL; - free(p->name); - free(p); - p = p1; - } - pkg->head = pkg->tail = NULL; } /* @@ -196,82 +212,86 @@ int plist_cmd(const char *s, char **arg) { - /* XXX (gcooper): this can blow up really quickly with the recent - * modifications made to read_plist, provided a sufficiently large list. */ - char cmd[FILENAME_MAX + 20]; /* 20 == fudge for max cmd len */ - char *cp; - const char *sp; + /* + * XXX (gcooper): this can blow up really quickly with the recent + * modifications made to read_plist, provided a sufficiently large + * list. + */ + char cmd[FILENAME_MAX + 20]; /* 20 == fudge for max cmd len */ + char *cp; + const char *sp; - /* - * FIXME (gcooper): this should be dynamic according to whatever's passed - * in. - */ - if (strlcpy(cmd, s, sizeof(cmd)) >= sizeof(cmd)) { - warnx("%s: line '%s' exceeds set limits", __func__, s); - errno = EINVAL; - return -1; - } - str_lowercase(cmd); - cp = cmd; - sp = s; - while (*cp) { - if (isspace(*cp)) { - *cp = '\0'; - while (isspace(*sp)) /* Never sure if macro, increment later */ - ++sp; - break; + /* + * FIXME (gcooper): this should be dynamic according to whatever's + * passed in. + */ + if (strlcpy(cmd, s, sizeof(cmd)) >= sizeof(cmd)) { + warnx("%s: line '%s' exceeds set limits", __func__, s); + errno = EINVAL; + return (-1); } - ++cp, ++sp; - } - if (arg) - *arg = (char *)sp; - if (!strcmp(cmd, "cwd")) - return PLIST_CWD; - else if (!strcmp(cmd, "srcdir")) - return PLIST_SRC; - else if (!strcmp(cmd, "cd")) - return PLIST_CWD; - else if (!strcmp(cmd, "exec")) - return PLIST_CMD; - else if (!strcmp(cmd, "unexec")) - return PLIST_UNEXEC; - else if (!strcmp(cmd, "mode")) - return PLIST_CHMOD; - else if (!strcmp(cmd, "owner")) - return PLIST_CHOWN; - else if (!strcmp(cmd, "group")) - return PLIST_CHGRP; - else if (!strcmp(cmd, "noinst")) - return PLIST_NOINST; - else if (!strcmp(cmd, "comment")) { - if (!strncmp(*arg, "ORIGIN:", 7)) { - *arg += 7; - return PLIST_ORIGIN; - } else if (!strncmp(*arg, "DEPORIGIN:", 10)) { - *arg += 10; - return PLIST_DEPORIGIN; + str_lowercase(cmd); + cp = cmd; + sp = s; + while (*cp != '\0') { + if (isspace(*cp)) { + *cp = '\0'; + /* Never sure if macro, increment later */ + while (isspace(*sp++)) ; + break; + } + cp++; + sp++; } - return PLIST_COMMENT; - } else if (!strcmp(cmd, "ignore")) - return PLIST_IGNORE; - else if (!strcmp(cmd, "ignore_inst")) - return PLIST_IGNORE_INST; - else if (!strcmp(cmd, "name")) - return PLIST_NAME; - else if (!strcmp(cmd, "display")) - return PLIST_DISPLAY; - else if (!strcmp(cmd, "pkgdep")) - return PLIST_PKGDEP; - else if (!strcmp(cmd, "conflicts")) - return PLIST_CONFLICTS; - else if (!strcmp(cmd, "mtree")) - return PLIST_MTREE; - else if (!strcmp(cmd, "dirrm")) - return PLIST_DIR_RM; - else if (!strcmp(cmd, "option")) - return PLIST_OPTION; - else - return -1; + if (arg) + *arg = (char *)sp; + if (!strcmp(cmd, "cwd")) + return (PLIST_CWD); + else if (!strcmp(cmd, "srcdir")) + return (PLIST_SRC); + else if (!strcmp(cmd, "cd")) + return (PLIST_CWD); + else if (!strcmp(cmd, "exec")) + return (PLIST_CMD); + else if (!strcmp(cmd, "unexec")) + return (PLIST_UNEXEC); + else if (!strcmp(cmd, "mode")) + return (PLIST_CHMOD); + else if (!strcmp(cmd, "owner")) + return (PLIST_CHOWN); + else if (!strcmp(cmd, "group")) + return (PLIST_CHGRP); + else if (!strcmp(cmd, "noinst")) + return (PLIST_NOINST); + else if (!strcmp(cmd, "comment")) { + if (!strncmp(*arg, "ORIGIN:", 7)) { + *arg += 7; + return (PLIST_ORIGIN); + } else if (!strncmp(*arg, "DEPORIGIN:", 10)) { + *arg += 10; + return (PLIST_DEPORIGIN); + } + return (PLIST_COMMENT); + } else if (!strcmp(cmd, "ignore")) + return (PLIST_IGNORE); + else if (!strcmp(cmd, "ignore_inst")) + return (PLIST_IGNORE_INST); + else if (!strcmp(cmd, "name")) + return (PLIST_NAME); + else if (!strcmp(cmd, "display")) + return (PLIST_DISPLAY); + else if (!strcmp(cmd, "pkgdep")) + return (PLIST_PKGDEP); + else if (!strcmp(cmd, "conflicts")) + return (PLIST_CONFLICTS); + else if (!strcmp(cmd, "mtree")) + return (PLIST_MTREE); + else if (!strcmp(cmd, "dirrm")) + return (PLIST_DIR_RM); + else if (!strcmp(cmd, "option")) + return (PLIST_OPTION); + else + return (-1); } /* Read a packing list from a file */ @@ -288,7 +308,7 @@ int serrno; int major; int minor; - int rc = -1; + int retcode = -1; off_t end_off; size_t len; @@ -303,10 +323,10 @@ 0)) != NULL) { end_off = sb.st_size; - rc = 0; + retcode = 0; start = contents_map; - while (rc == 0 && 0 < end_off) { + while (retcode == 0 && 0 < end_off) { end = strchr(start, '\n'); /* No trailing newlines -- look for '\0'. */ @@ -324,7 +344,7 @@ cmd_buf = malloc(end-start+1); if (cmd_buf == NULL) - rc = -1; + retcode = -1; else { strlcpy(cmd_buf, start, end-start+1); @@ -339,14 +359,14 @@ /* Empty line. */ if (len == 0) { errno = EINVAL; - rc = -1; + retcode = -1; } else cp = cmd_buf; } /* A plist command directive */ - if (rc == 0 && *start == CMD_CHAR) { + if (retcode == 0 && *start == CMD_CHAR) { cmd = plist_cmd(cmd_buf+1, &cp); @@ -354,15 +374,16 @@ warnx("%s: unknown command '%s' " "(package tools out of date?)", __func__, start); - rc = -1; + retcode = -1; } else if (*cp == '\0') { cp = NULL; if (cmd == PLIST_PKGDEP) { - warnx("corrupted record (pkgdep line " - "without argument), ignoring"); + warnx("corrupted record " + "(pkgdep line without " + "argument), ignoring"); errno = EINVAL; - cmd = rc = -1; + cmd = retcode = -1; } } @@ -388,7 +409,7 @@ if (pkg->fmtver_maj > PLIST_FMT_VER_MAJOR) { errno = EINVAL; - rc = -1; + retcode = -1; } } @@ -397,14 +418,14 @@ } /* A file manifest item */ - else if (rc == 0) + else if (retcode == 0) cmd = PLIST_FILE; /* * Winner, winner, chicken dinner.. we have a working * command! */ - if (rc == 0) { + if (retcode == 0) { add_plist(pkg, cmd, cp); start = end; @@ -436,14 +457,14 @@ errno = serrno; } - if (rc == -1 && cmd_buf != NULL) { + if (retcode == -1 && cmd_buf != NULL) { serrno = errno; free(cmd_buf); if (serrno == 0) errno = serrno; } - return rc; + return (retcode); } @@ -451,8 +472,9 @@ int write_plist(Package *pkg, FILE *fp) { + PackingList plist; - int rc = 0; + int retcode = 0; for (plist = pkg->head; plist != NULL; plist = plist->next) { @@ -543,19 +565,20 @@ break; case PLIST_DEPORIGIN: - fprintf(fp, "%ccomment DEPORIGIN:%s\n", CMD_CHAR, plist->name); + fprintf(fp, "%ccomment DEPORIGIN:%s\n", CMD_CHAR, + plist->name); break; default: warnx("%s: unknown command type %d (%s)", __func__, plist->type, plist->name); - rc = -1; + retcode = -1; break; } } - return rc; + return (retcode); } @@ -568,121 +591,166 @@ int delete_package(Boolean ign_err, Boolean nukedirs, Package *pkg) { - PackingList p; - const char *Where = ".", *last_file = ""; - Boolean fail = FALSE; - Boolean preserve; - char tmp[FILENAME_MAX], *name = NULL; - char *prefix = NULL; + PackingList p; + Boolean fail = FALSE; + Boolean preserve; + char tmp[FILENAME_MAX]; + char *name = NULL; + char *prefix = NULL; + const char *Where = "."; + const char *last_file = ""; + + preserve = find_plist_option(pkg, "preserve") ? TRUE : FALSE; + + for (p = pkg->head; p != NULL; p = p->next) { + + switch (p->type) { + case PLIST_NAME: + name = p->name; + break; + + case PLIST_IGNORE: + p = p->next; + break; + + case PLIST_CWD: + if (prefix == NULL) + prefix = p->name; + Where = (p->name == NULL) ? prefix : p->name; + if (Verbose) + printf("Change working directory to %s\n", + Where); + break; + + case PLIST_UNEXEC: + format_cmd(tmp, FILENAME_MAX, p->name, Where, + last_file); + if (Verbose) + printf("Execute '%s'\n", tmp); + if (!Fake && system(tmp)) { + warnx("unexec command for '%s' failed", tmp); + fail = -1; + } + break; + + /* + * TODO: break up this logic into more easily digestable + * blocks, if not to improve indentation, at least to improve + * modularity and testability via whitebox tests. + */ + case PLIST_FILE: + last_file = p->name; + sprintf(tmp, "%s/%s", Where, p->name); + /* TODO: set EISDIR */ + if (isdir(tmp) && fexists(tmp) && !issymlink(tmp)) { + warnx("cannot delete specified file '%s' - it " + "is a directory!\nthis packing list is " + "incorrect - ignoring delete request", + tmp); + } else { + if (p->next && + p->next->type == PLIST_COMMENT && + strncmp(p->next->name, "MD5:", 4) == 0) { + char *cp = NULL, buf[33]; + + /* + * For packing lists whose version is + * 1.1 or greater, the md5 hash for a + * symlink is calculated on the string + * returned by readlink(). + */ + if (issymlink(tmp) && + verscmp(pkg, 1, 0) > 0) { + char linkbuf[PATHNAME_MAX]; + int len; + + if ((len = readlink(tmp, + linkbuf, sizeof(linkbuf))) > 0) + cp = MD5Data((unsigned char *)linkbuf, len, buf); + } else if (isfile(tmp) || + verscmp(pkg, 1, 1) < 0) + cp = MD5File(tmp, buf); + + if (cp != NULL) { + + /* Mismatch? */ + if (strcmp(cp, + p->next->name + 4) != 0) { + warnx("'%s' fails " + "original MD5 " + "checksum - %s", + tmp, + (Force ? + "deleted anyway." : + "not deleted.")); + if (!Force) { + fail = -1; + continue; + } + + } - preserve = find_plist_option(pkg, "preserve") ? TRUE : FALSE; - for (p = pkg->head; p; p = p->next) { - switch (p->type) { - case PLIST_NAME: - name = p->name; - break; + } - case PLIST_IGNORE: - p = p->next; - break; + } - case PLIST_CWD: - if (prefix == NULL) - prefix = p->name; - Where = (p->name == NULL) ? prefix : p->name; - if (Verbose) - printf("Change working directory to %s\n", Where); - break; + if (Verbose) + printf("Delete file %s\n", tmp); + if (!Fake) { + if (delete_hierarchy(tmp, ign_err, + nukedirs)) + fail = -1; + if (preserve && name) { + char tmp2[FILENAME_MAX]; + + if (make_preserve_name(tmp2, + sizeof(tmp2), name, tmp)) { - case PLIST_UNEXEC: - format_cmd(tmp, FILENAME_MAX, p->name, Where, last_file); - if (Verbose) - printf("Execute '%s'\n", tmp); - if (!Fake && system(tmp)) { - warnx("unexec command for '%s' failed", tmp); - fail = -1; - } - break; + if (fexists(tmp2)) { + if (rename(tmp2, tmp)) + warn("preserve: unable to restore %s as %s", tmp2, tmp); + } - case PLIST_FILE: - last_file = p->name; - sprintf(tmp, "%s/%s", Where, p->name); - if (isdir(tmp) && fexists(tmp) && !issymlink(tmp)) { - warnx("cannot delete specified file '%s' - it is a directory!\n" - "this packing list is incorrect - ignoring delete request", tmp); - } - else { - if (p->next && p->next->type == PLIST_COMMENT && !strncmp(p->next->name, "MD5:", 4)) { - char *cp = NULL, buf[33]; + } - /* - * For packing lists whose version is 1.1 or greater, the - * md5 hash for a symlink is calculated on the string - * returned by readlink(). - */ - if (issymlink(tmp) && verscmp(pkg, 1, 0) > 0) { - int len; - char linkbuf[FILENAME_MAX]; + } - if ((len = readlink(tmp, linkbuf, FILENAME_MAX)) > 0) - cp = MD5Data((unsigned char *)linkbuf, len, buf); - } else if (isfile(tmp) || verscmp(pkg, 1, 1) < 0) - cp = MD5File(tmp, buf); + } - if (cp != NULL) { - /* Mismatch? */ - if (strcmp(cp, p->next->name + 4)) { - warnx("'%s' fails original MD5 checksum - %s", - tmp, Force ? "deleted anyway." : "not deleted."); - if (!Force) { - fail = -1; - continue; - } } - } - } - if (Verbose) - printf("Delete file %s\n", tmp); - if (!Fake) { - if (delete_hierarchy(tmp, ign_err, nukedirs)) - fail = -1; - if (preserve && name) { - char tmp2[FILENAME_MAX]; - - if (make_preserve_name(tmp2, FILENAME_MAX, name, tmp)) { - if (fexists(tmp2)) { - if (rename(tmp2, tmp)) - warn("preserve: unable to restore %s as %s", - tmp2, tmp); - } + + break; + + case PLIST_DIR_RM: + sprintf(tmp, "%s/%s", Where, p->name); + + if (!isdir(tmp) && fexists(tmp)) { + warnx("cannot delete specified directory '%s' " + "- it is a file!\nthis packing list is " + "incorrect - ignoring delete request", + tmp); + } else { + if (Verbose) + printf("Deleting directory %s\n", tmp); + if (Fake == FALSE && + delete_hierarchy(tmp, ign_err, FALSE)) { + warnx("unable to completely remove " + "directory '%s'", tmp); + fail = -1; + } } - } - } - } - break; + + last_file = p->name; + break; - case PLIST_DIR_RM: - sprintf(tmp, "%s/%s", Where, p->name); - if (!isdir(tmp) && fexists(tmp)) { - warnx("cannot delete specified directory '%s' - it is a file!\n" - "this packing list is incorrect - ignoring delete request", tmp); - } - else { - if (Verbose) - printf("Delete directory %s\n", tmp); - if (!Fake && delete_hierarchy(tmp, ign_err, FALSE)) { - warnx("unable to completely remove directory '%s'", tmp); - fail = -1; + default: + break; } - } - last_file = p->name; - break; - default: - break; } - } - return fail; + + return (fail); + } #ifdef DEBUG @@ -697,45 +765,73 @@ int delete_hierarchy(const char *dir, Boolean ign_err, Boolean nukedirs) { - char *cp1, *cp2; + + char *cp1, *cp2; + int retcode = 1; /* 1 -> retcode not touched. */ + + if (!fexists(dir) && !issymlink(dir)) { + if (ign_err == FALSE) { + warnx("%s '%s' doesn't exist", + isdir(dir) ? "directory" : "file", dir); + retcode = -1; + } + } else if (nukedirs) { + if (vsystem("%s -r%s %s", REMOVE_CMD, (ign_err ? "f" : ""), + dir)) + retcode = -1; + } else if (isdir(dir) && !issymlink(dir)) + if (RMDIR(dir) && !ign_err) + retcode = -1; + else + if (REMOVE(dir, ign_err)) + retcode = -1; + + /* + * If we got this far, strdup the paths as is required directly + * below. + */ + if (retcode == 1) { - cp1 = cp2 = strdup(dir); - if (!fexists(dir) && !issymlink(dir)) { - if (!ign_err) - warnx("%s '%s' doesn't exist", - isdir(dir) ? "directory" : "file", dir); - return !ign_err; - } - else if (nukedirs) { - if (vsystem("%s -r%s %s", REMOVE_CMD, (ign_err ? "f" : ""), dir)) - return 1; - } - else if (isdir(dir) && !issymlink(dir)) { - if (RMDIR(dir) && !ign_err) - return 1; - } - else { - if (REMOVE(dir, ign_err)) - return 1; - } + cp1 = cp2 = strdup(dir); + if (cp1 == NULL || cp2 == NULL) + retcode = -1; - if (!nukedirs) - return 0; - while (cp2) { - if ((cp2 = strrchr(cp1, '/')) != NULL) - *cp2 = '\0'; - if (!isemptydir(dir)) - return 0; - if (RMDIR(dir) && !ign_err) { - if (!fexists(dir)) - warnx("directory '%s' doesn't exist", dir); - else - return 1; } - /* back up the pathname one component */ - if (cp2) { - cp1 = strdup(dir); + + if (retcode == 1) { + + retcode = 0; + + if (nukedirs == TRUE) { + + while (cp2 != NULL && retcode == 0) { + + if ((cp2 = strrchr(cp1, '/')) != NULL) + *cp2 = '\0'; + if (!isemptydir(dir)) + retcode == 0; + if (RMDIR(dir) && !ign_err) { + + if (!fexists(dir)) + warnx("directory '%s' doesn't " + "exist", dir); + else + retcode = -1; + + } + + /* back up the pathname one component */ + if (cp2 != NULL) { + free(cp1); + cp1 = strdup(dir); + if (cp1 == NULL) + retcode = -1; + } + + } + } - } - return 0; + + return (retcode); + }
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201005310059.o4V0xs5q050262>