On Fri, 2009-01-16 at 13:38 -0500, Peter Shoults wrote: > I am putting this comment in last - ever file needs the copyright > updated to 2009. Ignore the few below that say copyright.
Yup. I'll update the year for all CRs. > >> 6768868 CPPFLAGS from Makefile.mech_krb5 get clobbered for pam_krb5 > >> and pam_krb5_migrate > >> http://cr.opensolaris.org/~mbp/6768868-wsdiff_pam_krb5/ > >> > Copyright again. Also, by putting Makefile.mech_krb5 after > Makefile.pam_modules, you will now lose, > CPPFLAGS += -D_REENTRANT that was defined in Makefile.pam_modules, > right? Is that ok, or should you > add the D_REENTRANT to the Makefile.mech_krb5? As Makefile.mech_krb5 only adds (+=) to CPPFLAGS any previously set CPPFLAGS will still be set including -D_REENTRANT. i.e this is ok. > > > >> 6776724 krb5_preauth_request_context_init leaks memory > >> http://cr.opensolaris.org/~mbp/6776724-PKINIT_memleaks/ > >> > So, for long-lived apps, are they going to keep re-opening and init'ing > the plugin info, which will keep growing memory usage? The plugin info will be re-inited but it won't leak. OpenSSL is smart enough not to leak memory in this case. It will initialize the global data once. I've also verified that this is the case. > If so, that > seems like another memory leak. See above. No new memory leak is introduced. > Copyright. > >> 6777148 idmap fails to auto-discover AD due to ldap_sasl_bind failure > >> http://cr.opensolaris.org/~mbp/6777148_init_creds_realm/ > >> > locate_kdc.c should call and check the results of the call to > krb5_is_referral_realm in the if statement using the same format as do > the other two files. Notably, locate_kdc.c uses a test of != TRUE, > which the others do not. The test in locate_kdc.c is a negative test for krb5_is_referral_realm(). If you'd like I can change it to: if (code && !krb5_is_referral_realm(realm)) { I don't mind either way. > > >> 6782682 krb5_recvauth() should return NULL for auth_context on failure > >> http://cr.opensolaris.org/~mbp/6782682-krb5_recv_auth_null/ > >> > >> > Hum - looking at this code, I do not see why this is right: > > 240 cleanup:; > 241 if (retval) { > 242 if (local_authcon) { > 243 krb5_auth_con_free(context, *auth_context); > 244 /* Solaris Kerberos */ > 245 *auth_context = NULL; > 246 } else if (local_rcache && rcache != NULL) { > 247 /* Solaris Kerberos */ > 248 (void) krb5_rc_close(context, rcache); > 249 krb5_auth_con_setrcache(context, *auth_context, NULL); > 250 } > 251 } > > I do not see why if we have an error, we could not have both > local_authcon set as well as having a local_rcache that needs to get > cleaned up. At first glance you seem to be correct. However thats not this bug. > It looks like if we have local_authcon, we will never test > and clean up the local_rcache if needed. > > > >> 6784485 keys for kadmind princs should be created with all supported > >> enc-types > >> http://cr.opensolaris.org/~mbp/6784485-enctypes_for_default_kadmin_princs/ > >> > > I am running out of time and have to leave soon. I will review this > last file on Monday if that is ok, or someone else can do the last one. No problem. Thanks for the review so far! -M
