On Tue, Aug 14, 2018 at 03:34:53PM +0300, Lauri Tirkkonen wrote:
> Similar to previous diffs for join(1) and paste(1). No intended
> functional change.
I agree with the direction. Comments inline.
> diff --git a/bin/md5/md5.c b/bin/md5/md5.c
> index 84adbcf0989..a8f18262a92 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,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) {
> 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';
You can just use linelen directly and skip the assignment.
> + 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.
To see this in action, make a cooked checklist file like this:
echo -n ' ' > checklist
md5 /bin/md5 >> checklist
perl -e 'print " "x256' >> checklist
md5 /bin/md5 >> checklist
and then rerun your patched md5 against the checklist with a
stricter malloc.conf(5):
MALLOC_OPTIONS=S md5 -C checklist /bin/md5 /bin/md5
Here, I'm seeing:
(MD5) /bin/md5: OK
md5(11805) in recallocarray(): modified chunk-pointer 0x1026a5f64581
Abort trap (core dumped)
The fix is easy though. Introduce an additional variable to keep track
of the first non-whitespace character instead of using line.
>
> /*
> * 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(line, ' ');
> if (p != NULL && *(p + 1) == '(') {
> /* BSD form */
> *p = '\0';
> - algorithm = buf;
> + algorithm = line;
> 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 = line;
> if ((p = strchr(checksum, ' ')) == NULL)
> continue;
> if (hf->style == STYLE_CKSUM) {
> @@ -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.
> 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
>
