[PATCH 2/3] core_pattern: allow passing of arguments to user mode helper when core_pattern is a pipe

[PATCH 2/3] core_pattern: allow passing of arguments to user mode helper when core_pattern is a pipe

[Neil Horman]

Hey
	Patch 2/3 of my core_pattern enhancements.  This patch is a rewrite of
my previous post for this enhancement.  It uses jeremy's split_argv/free_argv
library functions to translate core_pattern into an argv array to be passed to
the user mode helper process.  It also adds a translation to format_corename
such that the origional value of RLIMIT_CORE can be passed to userspace as an
argument.

Thanks & Regards
Neil

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>


 exec.c |   25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)


diff --git a/fs/exec.c b/fs/exec.c
index c0b5def..6855a52 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -51,6 +51,7 @@
 #include <linux/cn_proc.h>
 #include <linux/audit.h>
 #include <linux/signalfd.h>
+#include <linux/string.h>
 
 #include <asm/uaccess.h>
 #include <asm/mmu_context.h>
@@ -1543,6 +1544,14 @@ static int format_corename(char *corename, const char *pattern, long signr)
 					goto out;
 				out_ptr += rc;
 				break;
+			/* core limit size */
+			case 'c':
+				rc = snprintf(out_ptr, out_end - out_ptr,
+					      "%lu", current->signal->rlim[RLIMIT_CORE].rlim_cur); 
+				if (rc > out_end - out_ptr)
+					goto out;
+				out_ptr += rc;
+				break;
 			default:
 				break;
 			}
@@ -1727,6 +1736,9 @@ int do_coredump(long signr, int exit_code, struct pt_regs * regs)
 	int flag = 0;
 	int ispipe = 0;
 	unsigned long core_limit = current->signal->rlim[RLIMIT_CORE].rlim_cur;
+	char **helper_argv = NULL;
+	int helper_argc = 0;
+	char *delimit;
 
 	audit_core_dumps(signr);
 
@@ -1775,14 +1787,18 @@ int do_coredump(long signr, int exit_code, struct pt_regs * regs)
 	 * at which point file size limits and permissions will be imposed 
 	 * as it does with any other process
 	 */
-	if ((!ispipe) &&
-	   (core_limit < binfmt->min_coredump))
+	if ((!ispipe) && (core_limit < binfmt->min_coredump))
 		goto fail_unlock;
 
  	if (ispipe) {
 		core_limit = RLIM_INFINITY;
+		helper_argv = argv_split(GFP_KERNEL, corename+1, &helper_argc);
+		/* Terminate the string before the first option */
+		delimit = strchr(corename, ' ');
+		if (delimit)
+			*delimit = '\0';
 		/* SIGPIPE can happen, but it's just never processed */
- 		if(call_usermodehelper_pipe(corename+1, NULL, NULL, &file)) {
+ 		if(call_usermodehelper_pipe(corename+1, helper_argv, NULL, &file)) {
  			printk(KERN_INFO "Core dump to %s pipe failed\n",
 			       corename);
  			goto fail_unlock;
@@ -1817,6 +1833,9 @@ int do_coredump(long signr, int exit_code, struct pt_regs * regs)
 close_fail:
 	filp_close(file, NULL);
 fail_unlock:
+	if (helper_argv)
+		argv_free(helper_argv);
+
 	current->fsuid = fsuid;
 	complete_all(&mm->core_done);
 fail:
-- 
/***************************************************
 *Neil Horman
 *Software Engineer
 *Red Hat, Inc.
 *nhorman@tuxdriver.com
 *gpg keyid: 1024D / 0x92A74FA1
 *http://pgp.mit.edu
 ***************************************************/

Re: [PATCH 2/3] core_pattern: allow passing of arguments to user mode helper when core_pattern is a pipe

[Jeremy Fitzhardinge]

Neil Horman wrote:
> +	int helper_argc = 0;
>   
> +		helper_argv = argv_split(GFP_KERNEL, corename+1, &helper_argc);
>   

Hm, I suspect most users of argv_split don't really care about argc, so
it would useful to change argv_split to take NULL as the argc pointer,
rather than declare a bunch of unused variables.  Interested in throwing
a patch together?

    J

Re: [PATCH 2/3] core_pattern: allow passing of arguments to user mode helper when core_pattern is a pipe

[Neil Horman]

On Fri, Jul 27, 2007 at 01:54:19PM -0700, Jeremy Fitzhardinge wrote:
> Neil Horman wrote:
> > +	int helper_argc = 0;
> >   
> > +		helper_argv = argv_split(GFP_KERNEL, corename+1, &helper_argc);
> >   
> 
> Hm, I suspect most users of argv_split don't really care about argc, so
> it would useful to change argv_split to take NULL as the argc pointer,
> rather than declare a bunch of unused variables.  Interested in throwing
> a patch together?
> 
>     J


Gladly, I'll take care of it next week.

Regards
Neil

Re: [PATCH 2/3] core_pattern: allow passing of arguments to user mode helper when core_pattern is a pipe

[Martin Pitt]

[-- Attachment #1: Type: text/plain, Size: 1549 bytes --]

Hi Neil,

thanks a lot for your work on this!

Neil Horman [2007-07-27 16:08 -0400]:
> Hey
> 	Patch 2/3 of my core_pattern enhancements.  This patch is a rewrite of
> my previous post for this enhancement.  It uses jeremy's split_argv/free_argv
> library functions to translate core_pattern into an argv array to be passed to
> the user mode helper process.  
> [...]
>   	if (ispipe) {
>  		core_limit = RLIM_INFINITY;
> +		helper_argv = argv_split(GFP_KERNEL, corename+1, &helper_argc);

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.

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 would love to use macros instead since it looks a bit cleaner, and
personally I do not care about the "executable name" macro for Apport
[1] (I grab it from /proc/pid/exe), but I wanted to mention this
possible caveat before it goes upstream.

Thank you,

Martin

[1] https://wiki.ubuntu.com/Apport

-- 
Martin Pitt        http://www.piware.de
Ubuntu Developer   http://www.ubuntu.com
Debian Developer   http://www.debian.org

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 2/3] core_pattern: allow passing of arguments to user mode helper when core_pattern is a pipe

[Neil Horman]

On Sat, Jul 28, 2007 at 11:23:55AM +0200, Martin Pitt wrote:
> Hi Neil,
> 
> thanks a lot for your work on this!
> 
> Neil Horman [2007-07-27 16:08 -0400]:
> > Hey
> > 	Patch 2/3 of my core_pattern enhancements.  This patch is a rewrite of
> > my previous post for this enhancement.  It uses jeremy's split_argv/free_argv
> > library functions to translate core_pattern into an argv array to be passed to
> > the user mode helper process.  
> > [...]
> >   	if (ispipe) {
> >  		core_limit = RLIM_INFINITY;
> > +		helper_argv = argv_split(GFP_KERNEL, corename+1, &helper_argc);
> 
> 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.

> 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 would love to use macros instead since it looks a bit cleaner, and
> personally I do not care about the "executable name" macro for Apport
> [1] (I grab it from /proc/pid/exe), but I wanted to mention this
> possible caveat before it goes upstream.
> 
If you don't want to use a given macro, fine, don't use it.  If you want to use
a macro, thats fine too, but I really don't want to offer the ability to pass
macro's in core_pattern (it really just makes the parsing too hairy and prone to
error).  If you want to pass macros to apport, or whatever, we'll just let the
user mode helper inherit the crashing processes environment.  Its much easier
work, just as usefull, and more importantly, can be done later as a separate
piece of work.

Regards
Neil


> Thank you,
> 
> Martin
> 
> [1] https://wiki.ubuntu.com/Apport
> 
> -- 
> Martin Pitt        http://www.piware.de
> Ubuntu Developer   http://www.ubuntu.com
> Debian Developer   http://www.debian.org



-- 
/***************************************************
 *Neil Horman
 *Software Engineer
 *Red Hat, Inc.
 *nhorman@tuxdriver.com
 *gpg keyid: 1024D / 0x92A74FA1
 *http://pgp.mit.edu
 ***************************************************/

Re: [PATCH 2/3] core_pattern: allow passing of arguments to user mode helper when core_pattern is a pipe

[Martin Pitt]

[-- Attachment #1: Type: text/plain, Size: 3603 bytes --]

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.

> > 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).

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:

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).

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'].

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.

I did not defend Ubuntu's usage of environment variables, on the
contrary. Using the standard macros is more explicit and elegant, and
I welcome that change. I just pointed out the reason why we chose the
environment variable approach initially.

I just wanted to mention this little problem for the sake of
correctness.

Thank you, and have a nice weekend!

Martin

-- 
Martin Pitt        http://www.piware.de
Ubuntu Developer   http://www.ubuntu.com
Debian Developer   http://www.debian.org

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 2/3] core_pattern: allow passing of arguments to user mode helper when core_pattern is a pipe

[Neil Horman]

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.

> I did not defend Ubuntu's usage of environment variables, on the
> contrary. Using the standard macros is more explicit and elegant, and
> I welcome that change. I just pointed out the reason why we chose the
> environment variable approach initially.
> 
Ah, my bad.  We're on the same page then, and I just misunderstood what you were
referring to when you said macro expansion. I thought you wanted to have
core_pattern translate things like $HOME and soforth, which can more esaily be
passed to things like apport via environment inheritence, which we can look at
at a later date.

> I just wanted to mention this little problem for the sake of
> correctness.
> 
> Thank you, and have a nice weekend!
> 
> Martin
> 


Thank you for clearing me up on this.  So it would seem we're ok with what we
have now, correct?  We just have a potential corner case to address, which I can
reasonably handle with a modification to split_argv, that I have a todo on next
week.

Thanks & Regards
Neil

> -- 
> Martin Pitt        http://www.piware.de
> Ubuntu Developer   http://www.ubuntu.com
> Debian Developer   http://www.debian.org



-- 
/***************************************************
 *Neil Horman
 *Software Engineer
 *Red Hat, Inc.
 *nhorman@tuxdriver.com
 *gpg keyid: 1024D / 0x92A74FA1
 *http://pgp.mit.edu
 ***************************************************/

Re: [PATCH 2/3] core_pattern: allow passing of arguments to user mode helper when core_pattern is a pipe

[Jeremy Fitzhardinge]

Neil Horman wrote:
> 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.
>   

No, please don't.  My original argv_split did that, and it was just way
too complex.  If you need complex quoting, you can always point it at a
shell script and handle it there.

    J

Re: [PATCH 2/3] core_pattern: allow passing of arguments to user mode helper when core_pattern is a pipe

[Neil Horman]

On Sat, Jul 28, 2007 at 03:52:02PM -0700, Jeremy Fitzhardinge wrote:
> Neil Horman wrote:
> > 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.
> >   
> 
> No, please don't.  My original argv_split did that, and it was just way
> too complex.  If you need complex quoting, you can always point it at a
> shell script and handle it there.
> 
>     J

Ok, well then, it seems this corner case is much too harry to just fix up
immediately.  Given that we certainly don't handle quoted strings now, and the
fact that this is a case that will almost never come up, and can be esaily
worked around, lets address it at some time after we get this base functionality
in place

Regards
Neil


-- 
/***************************************************
 *Neil Horman
 *Software Engineer
 *Red Hat, Inc.
 *nhorman@tuxdriver.com
 *gpg keyid: 1024D / 0x92A74FA1
 *http://pgp.mit.edu
 ***************************************************/

Re: [PATCH 2/3] core_pattern: allow passing of arguments to user mode helper when core_pattern is a pipe

[Aneesh Kumar K.V]

Neil Horman wrote:
> 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.
> 


One would also need to quote the expansion of %e  as Martin listed in the previous mail
So a %e should expand to \"my executable\". So that it get passed as single argument.

I guess we should only do it when "|" is specified in core pattern. Otherwise we would
have core file as 

core."my exectutable" 


-aneesh

Re: [PATCH 2/3] core_pattern: allow passing of arguments to user mode helper when core_pattern is a pipe

[Martin Pitt]

[-- Attachment #1: Type: text/plain, Size: 1631 bytes --]

Hi Neil,

Neil Horman [2007-07-28 13:21 -0400]:
> 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.

Oh, handling escaping and quoting is going to make it fairly
complicated, but sure, if you need that for other things, too, that
would solve the remaining case. I just wonder if, instead of
implementing escaping, it wouldn't be easier to first split on spaces
and then escape macros?

> Thank you for clearing me up on this.  So it would seem we're ok with what we
> have now, correct?  

Absolutely, yes.

> We just have a potential corner case to address, which I can
> reasonably handle with a modification to split_argv, that I have a
> todo on next week.

Right, it's really just for perfectionism. Spaces in executable names
are EBW anyway, and readlink()ing /proc/<pid>/exe is much more robust
anyway in terms of a small and orthogonal interface.

If the upstream kernel guys don't worry about it and consider it a
blocker for merging, I don't either. :-)

Thanks a lot,

Martin

-- 
Martin Pitt        http://www.piware.de
Ubuntu Developer   http://www.ubuntu.com
Debian Developer   http://www.debian.org

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 2/3] core_pattern: allow passing of arguments to user mode helper when core_pattern is a pipe

[Neil Horman]

On Sun, Jul 29, 2007 at 02:23:10PM +0530, Aneesh Kumar K.V wrote:
> 
> 
> Neil Horman wrote:
> >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.
> >
> 
> 
> One would also need to quote the expansion of %e  as Martin listed in the 
> previous mail
> So a %e should expand to \"my executable\". So that it get passed as single 
> argument.
> 
> I guess we should only do it when "|" is specified in core pattern. 
> Otherwise we would
> have core file as 
> 
> core."my exectutable" 
> 

Martin and I have already gone over this in a previous post and I think we agree
that this is enough of a corner case, and so easily worked around, that we can
address it in a later patch.

Neil

> 
> -aneesh

-- 
/***************************************************
 *Neil Horman
 *Software Engineer
 *Red Hat, Inc.
 *nhorman@tuxdriver.com
 *gpg keyid: 1024D / 0x92A74FA1
 *http://pgp.mit.edu
 ***************************************************/

Re: [PATCH 2/3] core_pattern: allow passing of arguments to user mode helper when core_pattern is a pipe

[Neil Horman]

On Sun, Jul 29, 2007 at 11:34:18AM +0200, Martin Pitt wrote:
> Hi Neil,
> 
> Neil Horman [2007-07-28 13:21 -0400]:
> > 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.
> 
> Oh, handling escaping and quoting is going to make it fairly
> complicated, but sure, if you need that for other things, too, that
> would solve the remaining case. I just wonder if, instead of
> implementing escaping, it wouldn't be easier to first split on spaces
> and then escape macros?
> 
If it were just this case, then yes.  But we also need to make this work
properly for when core_pattern is not a pipe.  So the macro expansion routine
needs to be generalized somewhat.  Certainly doable, it just takes a bit more
testing and care in coding.  I'll start looking into it in the upcomming week.

> > Thank you for clearing me up on this.  So it would seem we're ok with what we
> > have now, correct?  
> 
> Absolutely, yes.
> 
> > We just have a potential corner case to address, which I can
> > reasonably handle with a modification to split_argv, that I have a
> > todo on next week.
> 
> Right, it's really just for perfectionism. Spaces in executable names
> are EBW anyway, and readlink()ing /proc/<pid>/exe is much more robust
> anyway in terms of a small and orthogonal interface.
> 
Understood.

> If the upstream kernel guys don't worry about it and consider it a
> blocker for merging, I don't either. :-)
> 
Copy that

Thanks & Regards
Neil

> Thanks a lot,
> 
> Martin
> 
> -- 
> Martin Pitt        http://www.piware.de
> Ubuntu Developer   http://www.ubuntu.com
> Debian Developer   http://www.debian.org



-- 
/***************************************************
 *Neil Horman
 *Software Engineer
 *Red Hat, Inc.
 *nhorman@tuxdriver.com
 *gpg keyid: 1024D / 0x92A74FA1
 *http://pgp.mit.edu
 ***************************************************/

Re: [PATCH 2/3] core_pattern: allow passing of arguments to user mode helper when core_pattern is a pipe

[Eugene Teo]

Neil Horman wrote:
[...]
> +			/* core limit size */
> +			case 'c':
> +				rc = snprintf(out_ptr, out_end - out_ptr,
> +					      "%lu", current->signal->rlim[RLIMIT_CORE].rlim_cur); 

Trailing space.

[...]
> - 		if(call_usermodehelper_pipe(corename+1, NULL, NULL, &file)) {
> + 		if(call_usermodehelper_pipe(corename+1, helper_argv, NULL, &file)) {
   ^^^^^^^^^^^^^

Use tabs, and a missing space after '('.

Eugene

Re: [PATCH 2/3] core_pattern: allow passing of arguments to user mode helper when core_pattern is a pipe

[Neil Horman]

On Sun, Jul 29, 2007 at 09:03:29PM +0800, Eugene Teo wrote:
> Neil Horman wrote:
> [...]
> > +			/* core limit size */
> > +			case 'c':
> > +				rc = snprintf(out_ptr, out_end - out_ptr,
> > +					      "%lu", current->signal->rlim[RLIMIT_CORE].rlim_cur); 
> 
> Trailing space.
> 
> [...]
> > - 		if(call_usermodehelper_pipe(corename+1, NULL, NULL, &file)) {
> > + 		if(call_usermodehelper_pipe(corename+1, helper_argv, NULL, &file)) {
>    ^^^^^^^^^^^^^
> 
> Use tabs, and a missing space after '('.
> 
I think your reader is screwing up the patch.  The version that I sent, which I
have here, shows that as all tabs, no spaces.

As for the '(', yeah, that looks like its been there for awhile.  I'll clean it
all up when I address macro expansion, as per the conversation Martin and I have
been holding.

Regards
Neil
 
> Eugene

-- 
/***************************************************
 *Neil Horman
 *Software Engineer
 *Red Hat, Inc.
 *nhorman@tuxdriver.com
 *gpg keyid: 1024D / 0x92A74FA1
 *http://pgp.mit.edu
 ***************************************************/