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.
Both linelen and len are inaccurate for the purposes of strcmp(3) and friends
after you've stubbed out the newline, anyway, so definitely use linelen directly
when you stub out the newline and skip assigning to len.
As for strcmp(3), I'm unsure. b64_ntop() seems to NUL-terminate the result and
we're looking at the full checksum string, so a switch to strcmp(3) might make
sense. If it does make sense it should happen in a separate diff, though.
>
> > > + while (isspace((unsigned char)*line))
> > > + line++;
> >
> > Incrementing the returned pointer works for fgetln(3) because it
> > keeps track of its own buffer internally. But if you try the same
> > trick with getline(3) it may try to realloc your invalid pointer.
>
> Ah, of course. Stupid mistake. Fixed below.
No worries, I hadn't seen it before either. That lengthy
explanation was partly so I was sure I understood it myself.
> > > @@ -725,11 +720,11 @@ digest_filelist(const char *file, struct
> > > hash_function *defhash, int selcount,
> > > error = 1;
> > > }
> > > }
> > > + free(line);
> >
> > getline(3) can fail, too, so you need to check listfp for errors after
> > freeing line.
>
> So can fgetln(3), but fair enough.
Yeah it's missing, so we kill two birds with one stone by converting
the code :)
> 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).
More comments inline.
>
> diff --git a/bin/md5/md5.c b/bin/md5/md5.c
> index 84adbcf0989..11345bcaa2a 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;
> 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,16 @@ 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) {
> + char *tmpline = line;
You're already modifying the stack variable declarations up above, so
declare tmpline alongside the other char pointers.
> 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++;
> + len = (size_t)linelen;
> + if (line[len - 1] == '\n')
> + line[len - 1] = '\0';
> + while (isspace((unsigned char)*tmpline))
> + tmpline++;
>
> /*
> * Crack the line into an algorithm, filename, and checksum.
> @@ -603,11 +599,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 +654,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 +721,13 @@ digest_filelist(const char *file, struct hash_function
> *defhash, int selcount,
> error = 1;
> }
> }
> + if (ferror(listfp))
> + warn("%s: getline", file);
> + free(line);
Set error = 1 in the ferror case, too.
> 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
