[systemd-devel] [PATCH 2/2] detect-virt: dmi: fix mem leak

[Andrew Jones]

On Wed, Nov 04, 2015 at 11:46:22AM +0100, Lennart Poettering wrote:
> On Tue, 03.11.15 15:04, Andrew Jones (drjones at redhat.com) wrote:
> 
> > The variable 's' is still in scope until we exit the function. We
> > can't call read_one_line_file on it multiple times without calling
> > free in between.
> > ---
> >  src/basic/virt.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/basic/virt.c b/src/basic/virt.c
> > index 1e10fc755f201..f88bd04bd72ed 100644
> > --- a/src/basic/virt.c
> > +++ b/src/basic/virt.c
> > @@ -176,9 +176,10 @@ static int detect_vm_dmi(void) {
> >  
> >                  r = read_one_line_file(dmi_vendors[i], &s);
> >                  if (r < 0) {
> > -                        if (r == -ENOENT)
> > +                        if (r == -ENOENT) {
> > +                                free(s);
> >                                  continue;
> 
> Note that we use the _cleanup_free_ macro on "s", which uses gcc's
> "cleanup" attribute to automatically free the variable when it goes
> out of scope. An explicit free() is hence unnecessary. In fact, it
> would result in a double-free.
> 
> We use the "cleanup" logic pretty much everywhere in systemd, in order
> to keep the error paths manageable.

As I wrote in the commit message 's' is still in scope until we exit
the function, detect_vm_dmi(). Calling read_one_line_file multiple times
will reassign 's' with newly allocated buffers each time, losing
references to the previously allocated buffers. _cleanup_free_ will only
ensure the last buffer is freed (which is why I didn't put the free() at
the function exit, but rather above the continue.)

Thanks,
drew

> 
> Lennart
> 
> -- 
> Lennart Poettering, Red Hat