* Re: libattr - severe memory leaks from attr_copy_file()
@ 2009-02-19 7:04 Zdenek Prikryl
2009-02-19 16:31 ` Eric Sandeen
2009-02-20 0:33 ` Timothy Shimmin
0 siblings, 2 replies; 6+ messages in thread
From: Zdenek Prikryl @ 2009-02-19 7:04 UTC (permalink / raw)
To: xfs
[-- Attachment #1: Type: text/plain, Size: 833 bytes --]
> 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.
--
Zdenek Prikryl <zprikryl@redhat.com>
[-- Attachment #2: attr-2.4.43-leak.patch --]
[-- Type: text/plain, Size: 899 bytes --]
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;
[-- Attachment #3: Type: text/plain, Size: 121 bytes --]
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: libattr - severe memory leaks from attr_copy_file()
2009-02-19 7:04 libattr - severe memory leaks from attr_copy_file() Zdenek Prikryl
@ 2009-02-19 16:31 ` Eric Sandeen
2009-02-20 0:33 ` Timothy Shimmin
1 sibling, 0 replies; 6+ messages in thread
From: Eric Sandeen @ 2009-02-19 16:31 UTC (permalink / raw)
To: Zdenek Prikryl; +Cc: xfs
Zdenek Prikryl 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.
The patch you attached looks good to me, thanks.
-Eric
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: libattr - severe memory leaks from attr_copy_file()
2009-02-19 7:04 libattr - severe memory leaks from attr_copy_file() Zdenek Prikryl
2009-02-19 16:31 ` Eric Sandeen
@ 2009-02-20 0:33 ` Timothy Shimmin
1 sibling, 0 replies; 6+ messages in thread
From: Timothy Shimmin @ 2009-02-20 0:33 UTC (permalink / raw)
To: Zdenek Prikryl; +Cc: xfs
Hi Zdenek,
On Thu, Feb 19, 2009 at 6:04 PM, Zdenek Prikryl <zprikryl@redhat.com> 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@redhat.com>
>
>
> 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@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
>
>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 6+ messages in thread
* libattr - severe memory leaks from attr_copy_file()
@ 2009-02-17 11:50 Zdenek Prikryl
2009-02-17 12:04 ` Arkadiusz Miskiewicz
0 siblings, 1 reply; 6+ messages in thread
From: Zdenek Prikryl @ 2009-02-17 11:50 UTC (permalink / raw)
To: xfs
[-- Attachment #1: Type: text/plain, Size: 191 bytes --]
Hello,
libattr's function attr_copy_file() seems to be producing severe memory leaks.
I'm sending a patch which removes sources of leaks.
Regards.
--
Zdenek Prikryl <zprikryl@redhat.com>
[-- Attachment #2: attr-2.4.32-leak.patch --]
[-- Type: text/plain, Size: 1083 bytes --]
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;
@@ -140,10 +141,12 @@ fail:
quote_free (ctx, q);
}
- free(pattern);
+ if (pattern)
+ free(pattern);
if (file)
fclose(file);
- free(text);
+ if (text)
+ free(text);
free_attr_actions();
return -1;
}
[-- Attachment #3: Type: text/plain, Size: 121 bytes --]
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: libattr - severe memory leaks from attr_copy_file()
2009-02-17 11:50 Zdenek Prikryl
@ 2009-02-17 12:04 ` Arkadiusz Miskiewicz
2009-02-18 7:08 ` Timothy Shimmin
0 siblings, 1 reply; 6+ messages in thread
From: Arkadiusz Miskiewicz @ 2009-02-17 12:04 UTC (permalink / raw)
To: xfs
On Tuesday 17 of February 2009, Zdenek Prikryl wrote:
> - free(text);
> + if (text)
> + free(text);
free(NULL) is fine (on Linux at least)
--
Arkadiusz Miśkiewicz PLD/Linux Team
arekm / maven.pl http://ftp.pld-linux.org/
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: libattr - severe memory leaks from attr_copy_file()
2009-02-17 12:04 ` Arkadiusz Miskiewicz
@ 2009-02-18 7:08 ` Timothy Shimmin
0 siblings, 0 replies; 6+ messages in thread
From: Timothy Shimmin @ 2009-02-18 7:08 UTC (permalink / raw)
To: Arkadiusz Miskiewicz; +Cc: xfs
On Tue, Feb 17, 2009 at 11:04 PM, Arkadiusz Miskiewicz <arekm@maven.pl> wrote:
>
> On Tuesday 17 of February 2009, Zdenek Prikryl wrote:
> > - free(text);
> > + if (text)
> > + free(text);
>
> free(NULL) is fine (on Linux at least)
Exactly - in most implementations for free I would say NULL is just fine.
And the variable, text, is assigned straight away before any use,
so I missed where the problem is.
--Tim
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-02-20 0:40 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-02-19 7:04 libattr - severe memory leaks from attr_copy_file() Zdenek Prikryl
2009-02-19 16:31 ` Eric Sandeen
2009-02-20 0:33 ` Timothy Shimmin
-- strict thread matches above, loose matches on Subject: below --
2009-02-17 11:50 Zdenek Prikryl
2009-02-17 12:04 ` Arkadiusz Miskiewicz
2009-02-18 7:08 ` Timothy Shimmin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox