Skip Menu |
 

History Show all quoted text — Show full headers
# Tue Sep 28 08:42:33 2010 "Arlene Berry" <aberry@likewise.com> - Ticket created
Subject: S4U memory leak
Date: Mon, 27 Sep 2010 20:07:45 -0400
From: "Arlene Berry" <aberry@likewise.com>
To: <krb5-bugs@mit.edu>
Download (untitled) / with headers
text/plain 581B
Download (untitled) / with headers
text/html 5KiB

When using S4U, in src/lib/gssapi/krb5/s4u_gss_glue.c kg_compose_deleg_cred, it creates a new unique memory credential cache that is internal to the credential.  It needs to be destroyed when the credential is released.  However, krb5_gss_release_cred only closes the cache and it’s being leaked.  Under other circumstances the cache field points to a pre-existing credential cache that should not be destroyed.  There is nothing that indicates whether the cache should be destroyed, so to solve this we’ll add a flag to the credential structure unless you have a better solution.

# Wed Sep 29 13:39:57 2010 ghudson@mit.edu (Greg Hudson) - Correspondence added
Download (untitled) / with headers
text/plain 340B
The other solution I can imagine is creating an anonymous memory ccache
variant which destroys itself on exit. There would be implications for
the current implementation of krb5_cc_dup(), as well as any code which
tries to use the ccache name.

A flag in the gss-krb5 creds structure is probably more conservative, and
is fine for now.
# Wed Sep 29 14:19:29 2010 "Arlene Berry" <aberry@likewise.com> - Correspondence added
Date: Wed, 29 Sep 2010 13:18:04 -0400
From: "Arlene Berry" <aberry@likewise.com>
To: <krb5-bugs@mit.edu>
Subject: RE: [krbdev.mit.edu #6787] S4U memory leak
RT-Send-Cc:
Download (untitled) / with headers
text/plain 2.9KiB
I see the same issue in the forwarded credentials case also. Here is
our patch using a flag as applied to your trunk. Our version still has
code for gss_add_cred in lib/gssapi/krb5 which I touched also but it
appears to be dead code. This works for us but I am not sure whether
you want to solve it this way since it appears to be a design issue
rather than simply incorrect code.


Index: src/lib/gssapi/krb5/rel_cred.c
===================================================================
--- src/lib/gssapi/krb5/rel_cred.c (revision 24369)
+++ src/lib/gssapi/krb5/rel_cred.c (working copy)
@@ -56,7 +56,16 @@
/* ignore error destroying mutex */

if (cred->ccache)
- code1 = krb5_cc_close(context, cred->ccache);
+ {
+ if (cred->destroy_ccache)
+ {
+ code1 = krb5_cc_destroy(context, cred->ccache);
+ }
+ else
+ {
+ code1 = krb5_cc_close(context, cred->ccache);
+ }
+ }
else
code1 = 0;

Index: src/lib/gssapi/krb5/gssapiP_krb5.h
===================================================================
--- src/lib/gssapi/krb5/gssapiP_krb5.h (revision 24369)
+++ src/lib/gssapi/krb5/gssapiP_krb5.h (working copy)
@@ -173,6 +173,7 @@
unsigned int proxy_cred : 1;
unsigned int default_identity : 1;
unsigned int iakerb_mech : 1;
+ unsigned int destroy_ccache : 1;

/* keytab (accept) data */
krb5_keytab keytab;
Index: src/lib/gssapi/krb5/s4u_gss_glue.c
===================================================================
--- src/lib/gssapi/krb5/s4u_gss_glue.c (revision 24369)
+++ src/lib/gssapi/krb5/s4u_gss_glue.c (working copy)
@@ -227,6 +227,8 @@
if (code != 0)
goto cleanup;

+ cred->destroy_ccache = 1;
+
code = krb5_cc_initialize(context, cred->ccache,
cred->proxy_cred ?
impersonator_cred->name->princ
:
subject_creds->client);
Index: src/lib/gssapi/krb5/acquire_cred.c
===================================================================
--- src/lib/gssapi/krb5/acquire_cred.c (revision 24369)
+++ src/lib/gssapi/krb5/acquire_cred.c (working copy)
@@ -548,6 +548,7 @@
#ifndef LEAN_CLIENT
cred->keytab = NULL;
#endif /* LEAN_CLIENT */
+ cred->destroy_ccache = 0;
cred->ccache = NULL;

code = k5_mutex_init(&cred->lock);
Index: src/lib/gssapi/krb5/accept_sec_context.c
===================================================================
--- src/lib/gssapi/krb5/accept_sec_context.c (revision 24369)
+++ src/lib/gssapi/krb5/accept_sec_context.c (working copy)
@@ -252,6 +252,7 @@
/* cred->name already set */
cred->keytab = NULL; /* no keytab associated with this... */
cred->tgt_expire = creds[0]->times.endtime; /* store the end
time */
+ cred->destroy_ccache = 1;
cred->ccache = ccache; /* the ccache containing the credential
*/
ccache = NULL; /* cred takes ownership so don't destroy */
}
# Fri Oct 08 15:53:13 2010 lukeh@padl.com (Luke Howard) - Comments added
Download (untitled) / with headers
text/plain 101B
This looks OK, but instead of adding a bitfield to indicate a memory ccache, use
krb5_cc_get_type().
# Fri Oct 08 17:43:18 2010 ghudson@mit.edu (Greg Hudson) - Correspondence added
Download (untitled) / with headers
text/plain 230B
Show quoted text
> This looks OK, but instead of adding a bitfield to indicate a memory
> ccache, use krb5_cc_get_type().

Wouldn't that do the wrong thing if the cred ccache is a pre-existing
memory ccache which the caller still has interest in?
# Sat Oct 09 09:47:04 2010 lukeh@padl.com (Luke Howard) - Correspondence added
Download (untitled) / with headers
text/plain 297B
[ghudson - Fri Oct 8 17:43:18 2010]:

Show quoted text
> > This looks OK, but instead of adding a bitfield to indicate a memory
> > ccache, use krb5_cc_get_type().
>
> Wouldn't that do the wrong thing if the cred ccache is a pre-existing
> memory ccache which the caller still has interest in?

Hmm. Fair enough.
# Mon Oct 25 17:55:55 2010 ghudson@mit.edu (Greg Hudson) - Given to ghudson@mit.edu (Greg Hudson)
# Mon Oct 25 17:55:55 2010 ghudson@mit.edu (Greg Hudson) - Target_Version 1.9 added
# Mon Oct 25 17:55:55 2010 ghudson@mit.edu (Greg Hudson) - Status changed from 'new' to 'review'
# Mon Oct 25 17:55:55 2010 ghudson@mit.edu (Greg Hudson) - Tags pullup added
# Mon Oct 25 17:55:55 2010 ghudson@mit.edu (Greg Hudson) - Correspondence added
From: ghudson@mit.edu
Subject: SVN Commit
Download (untitled) / with headers
text/plain 549B

When we create a temporary memory ccache for use within a
krb5_gss_cred_id_rec, set a flag to indicate that the ccache should be
destroyed rather than closed. Patch from aberry@likewise.com.


https://github.com/krb5/krb5/commit/d97562fd4e735509c86cfd94588bebf3240f8dde
Commit By: ghudson
Revision: 24482
Changed Files:
U trunk/src/lib/gssapi/krb5/accept_sec_context.c
U trunk/src/lib/gssapi/krb5/acquire_cred.c
U trunk/src/lib/gssapi/krb5/gssapiP_krb5.h
U trunk/src/lib/gssapi/krb5/rel_cred.c
U trunk/src/lib/gssapi/krb5/s4u_gss_glue.c
# Mon Nov 01 16:36:29 2010 tlyu (Taylor Yu) - Status changed from 'review' to 'resolved'
# Mon Nov 01 16:36:29 2010 tlyu (Taylor Yu) - Version_Fixed 1.9 added
# Mon Nov 01 16:36:29 2010 tlyu (Taylor Yu) - Correspondence added
From: tlyu@mit.edu
Subject: SVN Commit
Download (untitled) / with headers
text/plain 835B

pull up r24482 from trunk

------------------------------------------------------------------------
r24482 | ghudson | 2010-10-25 17:55:54 -0400 (Mon, 25 Oct 2010) | 8 lines

ticket: 6787
target_version: 1.9
tags: pullup

When we create a temporary memory ccache for use within a
krb5_gss_cred_id_rec, set a flag to indicate that the ccache should be
destroyed rather than closed. Patch from aberry@likewise.com.

https://github.com/krb5/krb5/commit/69e4bc941d1a652d46f7bec03e7ea0106496f44a
Commit By: tlyu
Revision: 24497
Changed Files:
U branches/krb5-1-9/src/lib/gssapi/krb5/accept_sec_context.c
U branches/krb5-1-9/src/lib/gssapi/krb5/acquire_cred.c
U branches/krb5-1-9/src/lib/gssapi/krb5/gssapiP_krb5.h
U branches/krb5-1-9/src/lib/gssapi/krb5/rel_cred.c
U branches/krb5-1-9/src/lib/gssapi/krb5/s4u_gss_glue.c
# Wed Dec 16 18:02:56 2015 tlyu (Taylor Yu) - CustomField pullup deleted