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 <[email protected]> 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 > -- ## List details at https://lists.exim.org/mailman/listinfo/exim-dev Exim details at http://www.exim.org/ ##
