On Sat, Jul 28, 2007 at 06:17:25PM +0200, Martin Pitt wrote:
Hi Neil,
Neil Horman [2007-07-28 9:46 -0400]:
I just want to mention a potential problem with this: If you first
expand the macros (from pattern to corename) and then split
corename into an argv, then this breaks macro expansions
containing spaces. This mostly affects the executable name, of
course.
I never intended for this core_pattern argument passing to be able
to expand macros, other than the macros specified by the
core_pattern code. If you want it to do that, we can address that
with a later patch.
No, I specifically mean the standard ones provided by
format_corename(), such as %p (pid), %s (signal), or %e (executable
name). I don't see a reason to provide additional functionality.
Ahh, well then you should have nothing to worry about, this patch expands them
just fine. And none of those macros will ever have spaces in them. I suppose
it would be possible for the executable name to have spaces in it, but honestly,
thats going rather out of your way to do something that you arguably shouldn't
do anyway.
In fact we considered this macro approach when doing the original
patches in the Ubuntu kernel, but we eventually used environment
variables because they are much easier and more robust to
implement than doing a robust macro expansion (i. e. first split
core_pattern into an argv and then call the macro expansion for
each element).
I disagree. While it might be nice to be able to specify environment
variables as command line arguments, it would be much easier to just
let the core_pattern executable inherit the crashing processes
environment. we don't do that currently, but we easily could. That
way any information that the crashing process wants the dying
process to know can be inherited and vetted easily by apport (or
whatever the core_pattern points to). I'll do a patch later for
that if you don't like it.
I don't think that this will be necessary. After all, the crash
handler can read all the environment from /proc/<pid>/ (and that's
indeed what apport does to figure out relevant parts from the
environment like the locale).
Agreed, /proc/<pid of crashing process>/* will be available while the helper app
runs.
It seems we misunderstood each other, I don't expect or want any new
functionality in core_pattern. AN example might make it more clear:
I think you're correct, I misundersood you previously. Apologies.
The original problem that we are trying to solve is the current
behaviour of core_pattern expansion with pipes:
|/bin/crash --pid %p
would try to execute the program '/bin/crash --pid 1234' instead of
calling /bin/crash with ['--pid', '1234'] as argv, right? Your patch
achieves the latter by splitting the formatted core dump string into
an array (at spaces).
Yes, that is exactly correct.
I pointed out that this leads to problems when macro values contain
spaces. This currently affects hostname (%h) (although this really
should not happen in practice) and executable name (%e) (rare, but at
least valid). I. e. for an executable name "foo bar" your patch would
expand
|/bin/crash %e
to ['/bin/crash', 'foo', 'bar'] instead of ['/bin/crash', 'foo bar'].
Also correct. Thats a pretty big corner case, and I personally don't think an
executable name with spaces is/should be valid anyway, but it can be done.
Of course this is a corner case, and personally I don't really care.
I strive to keep the assumptions about the interface at a minimum, so
right now Apport's only required input is the core dump itself (over
stdin); signal and pid can be read from the environment, and if not
present, they are read from the core dump.
Jeremy asked that I make a patch next week to address split_argv's requirement
that the argc parameter be non-NULL. I'll be fixing that next week, and what I
can do is further enhance it such that it ignores spaces in quoted strings,
which should address the case that concerns you. I.E I can make split_argv
behave such that:
echo "|\"foo bar\" --pid %p" > /proc/sys/kernel/core_pattern
results in the following argv:
{{"foo bar"}, {"--pid"}, {"1234"}}
Which I think handles what you are looking for.
