2010-01-06 17:01:19

by Jiri Slaby

Subject: [PATCH 1/1] cred: fix memory leak

Stanse found a memory leak in prepare_exec_creds. tgcred is not
freed/assigned on all paths. Fix that.

I.e. unifdef tgcred and add kfree(tgcred); as it is initialized to
NULL already.

Signed-off-by: Jiri Slaby <[email protected]>
Cc: David Howells <[email protected]>
Cc: James Morris <[email protected]>
Cc: Serge Hallyn <[email protected]>
---
kernel/cred.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/cred.c b/kernel/cred.c
index dd76cfe..0e10f73 100644
--- a/kernel/cred.c
+++ b/kernel/cred.c
@@ -351,9 +351,7 @@ struct cred *prepare_exec_creds(void)
*/
struct cred *prepare_usermodehelper_creds(void)
{
-#ifdef CONFIG_KEYS
struct thread_group_cred *tgcred = NULL;
-#endif
struct cred *new;

#ifdef CONFIG_KEYS
@@ -363,8 +361,10 @@ struct cred *prepare_usermodehelper_creds(void)
#endif

new = kmem_cache_alloc(cred_jar, GFP_ATOMIC);
- if (!new)
+ if (!new) {
+ kfree(tgcred);
return NULL;
+ }

kdebug("prepare_usermodehelper_creds() alloc %p", new);

--
1.6.5.7

2010-01-06 17:06:56

by David Howells

Subject: Re: [PATCH 1/1] cred: fix memory leak

Jiri Slaby <[email protected]> wrote:

> Stanse found a memory leak in prepare_exec_creds. tgcred is not
> freed/assigned on all paths. Fix that.
>
> I.e. unifdef tgcred and add kfree(tgcred); as it is initialized to
> NULL already.
>
> Signed-off-by: Jiri Slaby <[email protected]>
> Cc: David Howells <[email protected]>
> Cc: James Morris <[email protected]>
> Cc: Serge Hallyn <[email protected]>

Acked-by: David Howells <[email protected]>

2010-01-06 17:26:00

by Serge E. Hallyn

Subject: Re: [PATCH 1/1] cred: fix memory leak

Quoting Jiri Slaby ([email protected]):
> Stanse found a memory leak in prepare_exec_creds. tgcred is not
> freed/assigned on all paths. Fix that.
>
> I.e. unifdef tgcred and add kfree(tgcred); as it is initialized to
> NULL already.

Does this compile with CONFIG_KEYS=n, , though? I don't see a dummy
define for struct thread_group_cred in cred.h. Should this patch add
one?

> Signed-off-by: Jiri Slaby <[email protected]>
> Cc: David Howells <[email protected]>
> Cc: James Morris <[email protected]>
> Cc: Serge Hallyn <[email protected]>
> ---
> kernel/cred.c | 6 +++---
> 1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/cred.c b/kernel/cred.c
> index dd76cfe..0e10f73 100644
> --- a/kernel/cred.c
> +++ b/kernel/cred.c
> @@ -351,9 +351,7 @@ struct cred *prepare_exec_creds(void)
> */
> struct cred *prepare_usermodehelper_creds(void)
> {
> -#ifdef CONFIG_KEYS
> struct thread_group_cred *tgcred = NULL;
> -#endif
> struct cred *new;
>
> #ifdef CONFIG_KEYS
> @@ -363,8 +361,10 @@ struct cred *prepare_usermodehelper_creds(void)
> #endif
>
> new = kmem_cache_alloc(cred_jar, GFP_ATOMIC);
> - if (!new)
> + if (!new) {
> + kfree(tgcred);
> return NULL;
> + }
>
> kdebug("prepare_usermodehelper_creds() alloc %p", new);
>
> --
> 1.6.5.7

2010-01-06 17:31:39

by Jiri Slaby

Subject: Re: [PATCH 1/1] cred: fix memory leak

On 01/06/2010 06:25 PM, Serge E. Hallyn wrote:
> Quoting Jiri Slaby ([email protected]):
>> Stanse found a memory leak in prepare_exec_creds. tgcred is not
>> freed/assigned on all paths. Fix that.
>>
>> I.e. unifdef tgcred and add kfree(tgcred); as it is initialized to
>> NULL already.
>
> Does this compile with CONFIG_KEYS=n, , though?

Yes and I guess it's due to no dereference of the pointer.

> Should this patch add one?

Hmm, I don't think so. The patch is ugly in the light of not having the
struct defined. I should come up with something where the CONFIG_KEYS is
left there.

thanks,
--
js