On Tue, Aug 30, 2016 at 05:56:30PM -0700, [email protected] wrote: > Abstract the SPAM_LIST as a generic REPLACE_LIST > > REPLACE_LIST can be used more generally as a list of pattern > match-replace settings. SPAM_LIST was a special case of this, so > spam handling has been been changed to use REPLACE_LIST instead, and > SPAM_LIST was removed. > > A generic function for performing a REPLACE_LIST replacement has > been added in mutt_apply_replace().
Hi David,
I just have two comments about mutt_apply_replace().
The first is general. Mutt uses fixed size buffers for a lot of things,
but I think this might be one of the cases where we don't want to use a
LONG_STRING fixed length buffer. I realize this makes the
implementation even more complex, so I don't insist on it, but just
wanted to mention it. (I would much rather have your patch and put that
on the TODO list to fix than wait for your free time situation to
improve!)
The second is a few possible buffer overruns, commented inline below:
> diff --git a/muttlib.c b/muttlib.c
> --- a/muttlib.c
> +++ b/muttlib.c
> @@ -1071,6 +1071,100 @@
> *p = '_';
> }
>
> +char *mutt_apply_replace (char *dbuf, size_t dlen, char *sbuf, REPLACE_LIST
> *rlist)
> +{
> + REPLACE_LIST *l;
> + static regmatch_t *pmatch = NULL;
> + static int nmatch = 0;
> + static char twinbuf[2][LONG_STRING];
> + int switcher = 0;
> + char *p;
> + int i, n;
> + int tlen = 0;
> + char *src, *dst;
> +
> + if (dbuf && dlen)
> + dbuf[0] = '\0';
> +
> + if (sbuf == NULL || *sbuf == '\0')
> + return dbuf;
> +
> + twinbuf[0][0] = '\0';
> + twinbuf[1][0] = '\0';
> + src = twinbuf[switcher];
> + dst = src;
> +
> + strncpy(src, sbuf, LONG_STRING-1);
> + src[LONG_STRING-1] = '\0';
> +
> + for (l = rlist; l; l = l->next)
> + {
> + /* If this pattern needs more matches, expand pmatch. */
> + if (l->nmatch > nmatch)
> + {
> + safe_realloc (&pmatch, l->nmatch * sizeof(regmatch_t));
> + nmatch = l->nmatch;
> + }
> +
> + if (regexec (l->rx->rx, src, l->nmatch, pmatch, 0) == 0)
> + {
> + tlen = 0;
> + switcher ^= 1;
> + dst = twinbuf[switcher];
> +
> + dprint (5, (debugfile, "mutt_apply_replace: %s matches %s\n", src,
> l->rx->pattern));
> +
> + /* Copy into other twinbuf with substitutions */
> + if (l->template)
> + {
> + for (p = l->template; *p; )
> + {
> + if (*p == '%')
> + {
> + p++;
> + if (*p == 'L')
> + {
> + p++;
> + strncpy(&dst[tlen], src, pmatch[0].rm_so);
I think it's possible to overrun dst if we have a rx->template something
like:
'$' -> "%L%L%L%L%L%L%L%L%L%L"
so I think we would want to cap the length of the copy at
MIN(pmatch[0].rm_so, LONG_STRING-tlen-1)
and increment tlen by that amount in the next line.
> + tlen += pmatch[0].rm_so;
> + }
> + else if (*p == 'R')
> + {
> + p++;
> + strncpy(&dst[tlen], &src[pmatch[0].rm_eo], LONG_STRING-tlen-1);
> + tlen += strlen(src) - pmatch[0].rm_eo;
To make sure tlen doesn't overrun dst, probably want something equivalent to
tlen += MIN(LONG_STRING-tlen-1, strlen(src) - pmatch[0].rm_eo)
> + }
> + else
> + {
> + n = strtoul(p, &p, 10); /* get subst number */
> + while (isdigit((unsigned char)*p)) /* skip subst token */
> + ++p;
> + for (i = pmatch[n].rm_so; (i < pmatch[n].rm_eo) && (tlen <
> LONG_STRING-1); i++)
> + dst[tlen++] = src[i];
> + }
> + }
> + else
> + dst[tlen++] = *p++;
This should be length checked too, or the literal text part of the
template could overrun dst.
> + }
> + }
> + dst[tlen] = '\0';
> + dprint (5, (debugfile, "mutt_apply_replace: subst %s\n", dst));
> + }
> + src = dst;
> + }
> +
> + if (dbuf)
> + {
> + strncpy(dbuf, dst, dlen-1);
> + dbuf[dlen-1] = '\0';
> + }
> + else
> + {
> + dbuf = strdup(dst);
> + }
> + return dbuf;
> +}
> +
>
--
Kevin J. McCarthy
GPG Fingerprint: 8975 A9B3 3AA3 7910 385C 5308 ADEF 7684 8031 6BDA
signature.asc
Description: PGP signature
