[I wrote this ages ago when catching up on unread list email and never
quite finished it. But if it doesn't leave +drafts now then... It's a
distraction from the current 1.7.x issue, but the list archive might
find it useful.]
Hi David,
If you want to try the attached patch to mhshowsbr.c, it works for me
with:
mhshow-show-application/pdf: %pmime_helper %F %s
"`fmttest -raw -format '%(decode{text})' '%{name}'`"
...
As far as whether this is the right approach, I'd like to hear from
others.
Replying to multiple branches here, having caught up.
I think correct quoting of the expansions is the right solution. As
Paul said of Ken's suggestion to nuke dodgy characters, it's not the
least surprising.
The existing code around uip/mhshowsbr.c's parse_display_string()
obviously has problems. In trying to deal with quoting it only spots
single quotes, not double or backticks, for example. A bit of it had
the right idea at one time regarding single quoting, and currently says
/* Escape existing quotes */
while ((pp = strchr (pp, '\'')) && buflen > 3) {
len = strlen (pp++);
if (quoted) {
/* Quoted. Let this quote close that quoting.
Insert an escaped quote to replace it and
another quote to reopen quoting, which will be
closed below. */
memmove (pp + 2, pp, len);
*pp++ = '\\';
*pp++ = '\'';
buflen -= 2;
bp += 2;
quoted = false;
Given the user wrote «'%{name}'», that expands to «'foo'bar'», the
comment is turning «'bar'» found in the quoted string into «'\''bar'»,
and that is correct. But the code changed after the comment was written
to no longer insert the second single quote. The comment didn't change,
nor did the buflen test at the top from 3 to 2.
Speaking of buflen, the code seems to struggle in places with tracking
what's going on. We start off...
static int
parse_display_string(CT ct, char *cp, int *xstdin, int *xlist,
char *file, char *buffer, size_t buflen, int multipart)
{
char *bp = buffer;
bp[0] = bp[buflen] = '\0';
That looks like an overflow, as a buffer[4] with buflen 4 would have
indexes [0, 4), but the caller ensures buflen isn't `buffer length', but
`maximum index'. Cognitively tracking this detracts attention, probably
leading to things like
len = strlen(bp);
bp += len;
buflen -= len;
*bp = '\0';
Finally, if the MIME's charset isn't the locale's, then another $MH
entry is consulted, e.g. `mhshow-charset-iso-8859-1', and its value is
another template. This time, snprintf(3) does the work so the only
escape is `%s' and we assume the user complies.
if (ct->c_termproc) {
char term[BUFSIZ];
strncpy(term, buffer, sizeof(term));
snprintf(buffer, buflen, ct->c_termproc, term);
}
But they need to make sure they don't attempt to quote that escape,
perhaps to pass it as a single argument, as it may already have quotes.
And if the main template's expansion was `foo | bar' then they need to
ensure their `charset' template can handle sh, e.g. not use «xterm -fn
iso-font -e %s».
BTW, buflen is actually used as a buffer length there, so shortening the
available space by one, or it would be except it's not the original
`maximum index' buflen passed into the function, but the whittled down
one as the buffer was consumed. This suggests if more than roughly half
the buffer was consumed that the result would be truncated when copied
back under sprintf().
My point is, years of edits, for enhancement and fixes, have grown a bit
of a monster. They also make the behaviour surprising sometimes, e.g.
if $MH has
mhshow-show-text/foo: foo %p %q
then `foo' is passed just `%q'. Neither is mentioned in mhshow(1), nor
does it say what happens to unknown escapes, but `%p' meant something
once so the code silently ignores it now, whereas never-used `%q' is the
default case of passing it on. Old ones should be removed, with
deprecation if they're still likely to be around.
And the addition of « "$@"» by argsplit() at the end of the string given
to sh's -c is a bad idea for this use. I realise no arguments are
passed to sh this time, but it can still be set by the template, e.g.
as a simple means of parsing output. This intentionally runs the second
word from grep's output, and then, unintentionally, the whole line.
mhshow-show-text/foo: set `grep %s subtypes`; $2;
Enough moaning. Having studied it to see what it's trying to do,
I'm just trying to point out more than '-quoting needs considering.
An alternative to escapes that expand to content is environment
variables. Uppercase, of course, not bastard ones. They can hold all
bytes bar NUL, and it's then up to the user to refer to them in a
suitable manner for their unpredictable content, either in the template
or the program that runs. Documentation could guide the user, but
they'd probably make mistakes sometimes.
--
Cheers, Ralph.
https://plus.google.com/+RalphCorderoy
--
Nmh-workers
https://lists.nongnu.org/mailman/listinfo/nmh-workers