Hi Phil,
I was actually able to get the force_command approach working after doing some more digging into exim guts to understand how things work a bit better. Its a much more simple approach.
https://github.com/bdraco/exim/commit/force_command
Please let me know what else you would like me to do to prepare this.
Thanks
-Nick
On Mar 30, 2013, at 10:48 PM, Phil Pennock <pdp@???> wrote:
> 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
>
