On Tue, Aug 14 2018 14:29:30 -0500, Scott Cheloha wrote: > On Tue, Aug 14, 2018 at 08:55:14PM +0300, Lauri Tirkkonen wrote: > > Hi, > > > > On Tue, Aug 14 2018 12:08:54 -0500, Scott Cheloha wrote: > > > > + len = (size_t)linelen; > > > > + if (line[len - 1] == '\n') > > > > + line[len - 1] = '\0'; > > > > > > You can just use linelen directly and skip the assignment. > > > > I did it this way because this bit later > > > > if (base64) > > cmp = strncmp(checksum, digest, len); > > > > could have been reached without assigning len (if the "could be GNU form" > > branch is taken). It looks like checksum and digest will always be null > > temrinated, but 'len' may be reduced to something less than linelen > > (eg. in the case that the line starts with whitespace). Maybe there's > > some reason to not use strcmp here? > > base64 is only set in the BSD form case, and in that case len is set to > the length of checksum before the comparison.
Thanks, I neglected to check when base64 is set. Your logic makes sense in that case, so I applied your suggestion below. > > But shouldn't we check *before* calling free to ensure errno is not > > clobbered? (although, getline.3's example code certainly does err(1, > > "getline"); after free()...) > > free(3) doesn't set errno. At least, not portably. I definitely wouldn't > worry about the possibility of there being a malloc implementation that > clobbers errno. > > The ordering (check or free first) doesn't matter in this case because you > aren't returning from the function early if you have an error on listfp, > but I think the intent of the example in the manpage is to prevent leaks > in such cases, e.g.: > > if (ferror(listfp)) { > warn("getline"); > return 1; > } > free(line); /* leak */ > > I would just stick to the manpage example in this case and keep the free(3) > as close as possible to the allocation (getline). Thanks for the explanation. Maybe I'm too used to not trusting the platform to do things that make sense (in the past I remember thinking that even a pathological ferror could modify errno; its manpage says it won't though so I could trust that). > > + while ((linelen = getline(&line, &linesize, listfp)) != -1) { > > + char *tmpline = line; > > You're already modifying the stack variable declarations up above, so > declare tmpline alongside the other char pointers. fixed. > > + if (ferror(listfp)) > > + warn("%s: getline", file); > > + free(line); > > Set error = 1 in the ferror case, too. fixed. diff --git a/bin/md5/md5.c b/bin/md5/md5.c index 84adbcf0989..1abf28cfa16 100644 --- a/bin/md5/md5.c +++ b/bin/md5/md5.c @@ -549,11 +549,11 @@ digest_filelist(const char *file, struct hash_function *defhash, int selcount, int found, base64, error, cmp, i; size_t algorithm_max, algorithm_min; const char *algorithm; - char *filename, *checksum, *buf, *p; + char *filename, *checksum, *line, *p, *tmpline; char digest[MAX_DIGEST_LEN + 1]; - char *lbuf = NULL; + ssize_t linelen; FILE *listfp, *fp; - size_t len, nread; + size_t len, linesize, nread; int *sel_found = NULL; u_char data[32 * 1024]; union ANY_CTX context; @@ -580,20 +580,15 @@ digest_filelist(const char *file, struct hash_function *defhash, int selcount, } error = found = 0; - while ((buf = fgetln(listfp, &len))) { + line = NULL; + linesize = 0; + while ((linelen = getline(&line, &linesize, listfp)) != -1) { + tmpline = line; base64 = 0; - if (buf[len - 1] == '\n') - buf[len - 1] = '\0'; - else { - if ((lbuf = malloc(len + 1)) == NULL) - err(1, NULL); - - (void)memcpy(lbuf, buf, len); - lbuf[len] = '\0'; - buf = lbuf; - } - while (isspace((unsigned char)*buf)) - buf++; + if (line[linelen - 1] == '\n') + line[linelen - 1] = '\0'; + while (isspace((unsigned char)*tmpline)) + tmpline++; /* * Crack the line into an algorithm, filename, and checksum. @@ -603,11 +598,11 @@ digest_filelist(const char *file, struct hash_function *defhash, int selcount, * Fallback on GNU form: * CHECKSUM FILENAME */ - p = strchr(buf, ' '); + p = strchr(tmpline, ' '); if (p != NULL && *(p + 1) == '(') { /* BSD form */ *p = '\0'; - algorithm = buf; + algorithm = tmpline; len = strlen(algorithm); if (len > algorithm_max || len < algorithm_min) continue; @@ -658,7 +653,7 @@ digest_filelist(const char *file, struct hash_function *defhash, int selcount, if ((hf = defhash) == NULL) continue; algorithm = hf->name; - checksum = buf; + checksum = tmpline; if ((p = strchr(checksum, ' ')) == NULL) continue; if (hf->style == STYLE_CKSUM) { @@ -725,11 +720,15 @@ digest_filelist(const char *file, struct hash_function *defhash, int selcount, error = 1; } } + free(line); + if (ferror(listfp)) { + warn("%s: getline", file); + error = 1; + } if (listfp != stdin) fclose(listfp); if (!found) warnx("%s: no properly formatted checksum lines found", file); - free(lbuf); if (sel_found != NULL) { /* * Mark found files by setting them to NULL so that we can -- Lauri Tirkkonen | lotheac @ IRCnet