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
