[systemd-devel] [PATCH 2/4] core: fix invalid free() in killall()

[Lennart Poettering]

On Sun, 15.06.14 07:51, Andrey Borzenkov (arvidjaar at gmail.com) wrote:

> В Fri, 13 Jun 2014 18:48:19 +0200
> Andreas Henriksson <andreas at fatal.se> пишет:
> 
> > static int killall(....) in ./src/core/killall.c tries to get "s"
> > initialized by calling get_process_comm(...) which calls
> > read_one_line_file(...) which if it fails will mean it is left
> > uninitialized.
> > It is then used in argument to strna(s) call where it is
> > dereferenced(!), in addition to nothing else initializing it before
> > the scope it is in finishes.
> > 
> > Signed-off-by: Andreas Henriksson <andreas at fatal.se>
> > ---
> >  src/core/killall.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/src/core/killall.c b/src/core/killall.c
> > index 57ed41c..eab48f7 100644
> > --- a/src/core/killall.c
> > +++ b/src/core/killall.c
> > @@ -168,7 +168,7 @@ static int killall(int sig, Set *pids, bool send_sighup) {
> >                          continue;
> >  
> >                  if (sig == SIGKILL) {
> > -                        _cleanup_free_ char *s;
> > +                        _cleanup_free_ char *s = NULL;
> >  
> >                          get_process_comm(pid, &s);
> 
> Do you mean this may fail to assign value to s? Then it needs more
> fixes.
> 
> >                          log_notice("Sending SIGKILL to PID "PID_FMT" (%s).", pid, strna(s));

Nah, the code is fine, now. THis is specifically written so that getting
the "comm" name is something we try, but ultimately don't care about. We
explicitly guard against failure by using strna() on the string we
acquired...

But of course, this was broken before Andreas' patches where the string
wasn't actually initialized to NULL...

Lennart

-- 
Lennart Poettering, Red Hat