Re: [exim-dev] use_wrapper patch

On 2013-03-29 at 22:33 -1000, J. Nick Koston wrote:
> It looks like $pipe_addresses is handled as a special case. I could
> just make $address_pipe a special case (just sounds icky though) and
> strip of the leading |, however I'm not familiar with code to fully
> understand the implications (it starts to make the use_wrapper idea
> look better).
>
> Would you be so kind as to provide some guidance?

Something about use_wrapper is tickling a "caution" nerve, and I can't
place it; perhaps it's just wrappers around wrappers, or perhaps it's
just annoyance that we still don't have lists as a first-class data
structure inside Exim, making this harder than it needs to be.

Let's proceed on the assumption that your original approach is the best
one and I'm just being apprehensive for no good reason.


For consistency with other evaluations of true/false, per expand.c, you
should also capture "no" as a scenario. Except don't: see below.

You don't appear to handle expansion to an empty string?

What does it mean when expansion failure is _forced_? Ie, if
expand_string_forcedfail is set?

Perhaps both "empty string" and "forced expansion failure" should be
equivalent to "false", ie do not use a wrapper. In fact, I think part
of the issue here is that two semantic interpretations are being
conflated. One is "this is the command to actually run, and other
options, before the real command", the other is "try to look like a
condition". While "false" is unlikely to be the wrapper, it is a real
command and overlaying the interpretation seems unwise.

So I suggest that either expand_string_forcedfail (explicit 'fail' in an
expansion) or an empty string should be taken as "don't use the
wrapper", and anything else is a real command.

This is more consistent with how Exim handles this sort of
bail-out-of-being-set scenario for other options.

You don't seem to do anything to resize argv to make sure it has space
for the extra option you insert; you _seem_, on my reading, to just be
assuming that there's space for one more pointer at the end, shifting
everything along by one (including the terminating NULL) and then
inserting the new option. Bad luck with memory layout could mean
that this is in fact overwriting other data and causing memory
corruption, albeit "only" blanking out 4/8 bytes with zeros for the
terminating NULL pointer. Still, that's memory corruption.

Have I missed something, which guarantees that the memory is actually
available for this shifting?

The documentation added doesn't mention that the result of the expansion
must be exactly _one_ parameter, which will be argv[0], instead of a
list of argv[] items to be prepended (my understanding from just reading
the spec change). It seems more useful to be able to insert a prefix
command and its options, but if you want to update the documentation to
be clearer that only one argv item may result, that works too.


Thanks,
-Phil