Mark Phalan wrote: > 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. >
Yeah - I think that is more uniform for all three files/routines. > >>>> 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. > Is that a different bug? If so, perhaps we should open on to track it. > >> 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 > > _______________________________________________ > kerberos-discuss mailing list > kerberos-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/kerberos-discuss >
