xfs
[Top] [All Lists]

Re: libattr - severe memory leaks from attr_copy_file()

To: Zdenek Prikryl <zprikryl@xxxxxxxxxx>
Subject: Re: libattr - severe memory leaks from attr_copy_file()
From: Timothy Shimmin <timothy.shimmin@xxxxxxxxx>
Date: Fri, 20 Feb 2009 11:33:13 +1100
Cc: xfs@xxxxxxxxxxx
Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:received:in-reply-to:references :date:message-id:subject:from:to:cc:content-type :content-transfer-encoding; bh=8Uzhu+sbPIeFjxmTBwQv7Wqmlj1ojiAQ3loB83mR5xI=; b=E9gcrRs2sxBX0V9lFpuUh7jQNdSa4rDp4hN27nntONlZ3uT6U2IwJnU4FcXsNKCdQ8 JTNJhM3tKZPtQPELG7k0NsDv3PUC5pHm5DlTebk1Fva0aq4o2POgnqyKRDYlPeMLHdFv W6kYxoL6ISPJMvNNptvxNBETw0TJYYM9dAIuc=
Domainkey-signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding; b=oXW2/fLAoJrW8HcV7Jrx1mj3PrOgiZaXWmapEcz5ReHiEBo6jSgiAcNR+pELcBtxWw qPEt+FKmNehjtOrU3S9d3vyejY7HIg+PeoB+bKepXaIW3lr2ujlHh66Z0IPnsJ3odDKA O6/QrapXPaqB/H1s6Hx7EeCkL7eso7I9S/Bpg=
In-reply-to: <499D0471.6000600@xxxxxxxxxx>
References: <499D0471.6000600@xxxxxxxxxx>
Hi Zdenek,

On Thu, Feb 19, 2009 at 6:04 PM, Zdenek Prikryl <zprikryl@xxxxxxxxxx> wrote:
>> And the variable, text, is assigned straight away before any use,
>> so I missed where the problem is.
>>
>> --Tim
>
> The memory leak is really there. Look:
:-)

>
> 54 attr_parse_attr_conf(struct error_context *ctx)
> ...
> 66 repeat:
> 67         text = malloc(size_guess + 1);
> 68         if (!text)
> 69                 goto fail;
> 70
> 71         if ((file = fopen(ATTR_CONF, "r")) == NULL) {
> 72                 if (errno == ENOENT)
> 73                         return 0;
> 74                 goto fail;
> 75         }
>
> Let's say that malloc() on the line 67 success, so we have text != NULL. Then,
> fopen() on the line 71 fails and errno == ENOENT. In that case
> attr_parse_attr_conf() simply returns 0, but text isn't freed. That's the 
> point,
> where memory leaks arise. I rewrote the patch, so now is more simpler.
>
Oh okay.
I see. I was looking at the "fail" case.
Hmmm.... the direct return case.
The simpler patch looks better - cool.

So I now worry about all the cases where we call return directly.
It looks like the code is building up an action list and string dup'ing
patterns.
So in the normal case where it processes them and then returns 0,
I can't see where it is free'ing the text.
Am I missing something again?

My other concern is the free(pattern) and the free(attr_actions->pattern)
with the possibility of trying to free the pattern twice. Probably, could set
pattern to NULL after it is put into the new action or some such.

--Tim


> --
> Zdenek Prikryl <zprikryl@xxxxxxxxxx>
>
>
> diff -up attr-2.4.43/libattr/attr_copy_action.c.leak 
> attr-2.4.43/libattr/attr_copy_action.c
> --- attr-2.4.43/libattr/attr_copy_action.c.leak 2008-06-30 07:22:50.000000000 
> +0200
> +++ attr-2.4.43/libattr/attr_copy_action.c      2009-02-17 09:50:38.000000000 
> +0100
> @@ -53,7 +53,7 @@ free_attr_actions(void)
>  static int
>  attr_parse_attr_conf(struct error_context *ctx)
>  {
> -       char *text, *t;
> +       char *text = NULL, *t;
>        size_t size_guess = 4096, len;
>        FILE *file;
>        char *pattern = NULL;
> @@ -64,15 +64,16 @@ attr_parse_attr_conf(struct error_contex
>                return 0;
>
>  repeat:
> -       text = malloc(size_guess + 1);
> -       if (!text)
> -               goto fail;
> -
>        if ((file = fopen(ATTR_CONF, "r")) == NULL) {
>                if (errno == ENOENT)
>                        return 0;
>                goto fail;
>        }
> +
> +       text = malloc(size_guess + 1);
> +       if (!text)
> +               goto fail;
> +
>        len = fread(text, 1, size_guess, file);
>        if (ferror(file))
>                goto fail;
>
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs
>
>

<Prev in Thread] Current Thread [Next in Thread>