Re: [systemd-devel] [PATCH 2/2] coredump: Handle programs with spaces in COMM

Re: [systemd-devel] [PATCH 2/2] coredump: Handle programs with spaces in COMM

[Oleg Nesterov]

On 04/30, Colin Walters wrote:
>
> On Tue, 2013-04-30 at 19:47 +0200, Zbigniew Jędrzejewski-Szmek wrote:
> > On Tue, Apr 30, 2013 at 01:12:19PM -0400, Colin Walters wrote:
> > > This patch makes systemd-coredump handle processes that have
> > > whitespace in their COMM fields.
> > >
> > > fs/coredump.c when given %e (as systemd-coredump uses), will end up
> > > joining the process arguments into a string (along with the other
> > > fields), then will split the entire thing up on whitespace, and use
> > > it as the arguments to the coredump pipe handler.
> > > ---
> > That's a workaround for a bug in the kernel. I think it makes sense, but
> > it'd be nice to fix the kernel too.

I wouldn't say this is bug... at least this is expected.

Sure, it is possible to rewrite format_corename/argv_split interaction,
but this is a bit painful and I am not sure it worth the trouble.

> To do what though?  Add a new coredump format specifier that gives you
> a string-escaped version as one argument?  That'd probably make sense,

Or, perhaps, we can simply change cn_escape() to do s/space/something/
unconditionally (currently it only does s'/'!'). But this is a user-
visible change.

Oleg.

Re: [systemd-devel] [PATCH 2/2] coredump: Handle programs with spaces in COMM

[Lennart Poettering]

On Wed, 01.05.13 18:42, Oleg Nesterov (oleg@redhat.com) wrote:

> On 04/30, Colin Walters wrote:
> >
> > On Tue, 2013-04-30 at 19:47 +0200, Zbigniew Jędrzejewski-Szmek wrote:
> > > On Tue, Apr 30, 2013 at 01:12:19PM -0400, Colin Walters wrote:
> > > > This patch makes systemd-coredump handle processes that have
> > > > whitespace in their COMM fields.
> > > >
> > > > fs/coredump.c when given %e (as systemd-coredump uses), will end up
> > > > joining the process arguments into a string (along with the other
> > > > fields), then will split the entire thing up on whitespace, and use
> > > > it as the arguments to the coredump pipe handler.
> > > > ---
> > > That's a workaround for a bug in the kernel. I think it makes sense, but
> > > it'd be nice to fix the kernel too.
> 
> I wouldn't say this is bug... at least this is expected.
> 
> Sure, it is possible to rewrite format_corename/argv_split interaction,
> but this is a bit painful and I am not sure it worth the trouble.

It sounds really wrong to first merge this into one string and then
split it up again. It sounds much more sensible to instead just pass the
string array around all the time. What's the reason to make this one
string first?

Lennart

-- 
Lennart Poettering - Red Hat, Inc.

Re: [systemd-devel] [PATCH 2/2] coredump: Handle programs with spaces in COMM

[Colin Walters]

On Fri, 2013-05-03 at 17:08 +0200, Lennart Poettering wrote:

> It sounds really wrong to first merge this into one string and then
> split it up again. It sounds much more sensible to instead just pass the
> string array around all the time. What's the reason to make this one
> string first?

I'm wondering if there are compatibility concerns; abrt wouldn't care
from what I can tell if we just changed the kernel.  systemd-coredump is
just plain broken right now.  I'll look for the source to the Ubuntu
one...

Re: [systemd-devel] [PATCH 2/2] coredump: Handle programs with spaces in COMM

[Oleg Nesterov]

Sorry for delay, vacation.

On 05/03, Lennart Poettering wrote:
>
> On Wed, 01.05.13 18:42, Oleg Nesterov (oleg@redhat.com) wrote:
>
> > I wouldn't say this is bug... at least this is expected.
> >
> > Sure, it is possible to rewrite format_corename/argv_split interaction,
> > but this is a bit painful and I am not sure it worth the trouble.
>
> It sounds really wrong to first merge this into one string and then
> split it up again. It sounds much more sensible to instead just pass the
> string array around all the time. What's the reason to make this one
> string first?

!ispipe case.

OK. I have to admit that this doesn't look nice even if this is
"historical" behaviour. The fix should be simple I guess,
format_corename() should construct argv by hand, argv_split()
should be avoided.

But:

	- We do not want to complicate this (ugly) code more than
	  necessary. In particular we shouldn't forget about ispipe.

	  Oh, and realloc, we can't count argc in advance or we need
	  more changes to protect against proc_dostring_coredump().

	- We should cleanup format_corename() first. If nothing else,
	  cn_escape().

	- Hmm. it seems that we also need to fix it, format_corename()
	  can leak ->corename afaics.

In short: I'll try to do this when I have time.

Oleg.