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/ ##
