On Wed, 2008-10-01 at 18:35 -0700, Glenn Barry wrote: > here's most of chunk 3. > > note most of the cast stuff i complain about might be better to fix it > on mit codebase and then get it in sol via later sync. > > > > http://cr.opensolaris.org/~mbp/pkinit/usr/src/lib/gss_mechs/mech_krb5/krb5/rcache/rcdef.c.wdiff.html > > 31 - * replay cache default set of operation vectors. > 33 + * replay cache default operationvectors. > > * should operationvectors be all one word?
No. As it doesn't change the code though it should be fixed upstream in MIT first. > > http://cr.opensolaris.org/~mbp/pkinit/usr/src/lib/gss_mechs/mech_krb5/mech/accept_sec_context.c.wdiff.html > > 268 - * SUNW15resync > 269 - * Added this cc_destroy for template_cache, w/out it > causes memory > 270 - * leak via "ssh -o gssapidelegatecredentials=yes ..." > 271 - */ > 272 - if (template_ccache) > 273 - (void)krb5_cc_destroy(context, template_ccache); > > * sure this is no longer needed? (do a ssh test and run findleaks on sshd) > This is no longer needed. See changes earlier in file removing "template_ccache". I did a test with ssh and I see no leaks when using the new code. > http://cr.opensolaris.org/~mbp/pkinit/usr/src/lib/gss_mechs/mech_krb5/mech/export_sec_context.c.wdiff.html > > 65 - { gss_OID go = ctx->mech_used; > 66 - printf("export ctx len=%lu\n", go->length); > 67 - } > > * oops :) > > http://cr.opensolaris.org/~mbp/pkinit/usr/src/lib/gss_mechs/mech_krb5/mech/init_sec_context.c.wdiff.html > > 1399 - /* > 1400 - * Solaris Kerberos: > 1401 - * If the client's realm is empty (using a fallback > method to determine > 1402 - * the realm) then make sure to set the client's > realm to the default > 1403 - * realm. This ensures that the TGT is built correctly. > 1404 - */ > 1405 - if (krb5_is_referral_realm(&(me->realm))) { > 1406 - char *realm; > 1407 1395 > 1408 - free(me->realm.data); > 1409 - code = krb5_get_default_realm(context, &realm); > 1410 - if (code) { > 1411 - (void) krb5_kt_close(context, keytab); > 1412 - *minor_status = code; > 1413 - return (GSS_S_FAILURE); > 1414 - } > 1415 - > 1416 - me->realm.data = realm; > 1417 - me->realm.length = strlen(realm); > > * it's: 6523887 krb should support client side referrals > no longer needed? The above workaround is no longer needed when using referrals. > > http://cr.opensolaris.org/~mbp/pkinit/usr/src/lib/gss_mechs/mech_krb5/profile/prof_file.c.wdiff.html > > * yes! lots of unnecessary casts removed from malloc calls, one of > my pet peeves! > > http://cr.opensolaris.org/~mbp/pkinit/usr/src/lib/gss_mechs/mech_krb5/profile/prof_parse.c.wdiff.html > > 325 - if (!str || !*str) > 326 - return 0; > 320 + if (!str) > 321 + return 0; > 322 + if (*str) > 323 + return 1; > > * is 322,3 correct? > Nope. Fixed. > > http://cr.opensolaris.org/~mbp/pkinit/usr/src/lib/krb5/kadm5/clnt/chpw.c.wdiff.html > 72 + memcpy(ptr, ap_req->data, ap_req->length); > 73 + ptr += ap_req->length; > 74 + > 75 + /* krb-priv of password */ > 76 + > 77 + memcpy(ptr, cipherpw.data, cipherpw.length); > > * memcpy returns void* so needs void cast here > (see others too) This comes back to lint issues. What's is the requirement? That nightly with the lint flag comes back clean (it does for this code)? > > 239 + result_data->data = (char *) malloc(result_data->length); > > * malloc does not need cast MIT has it so I'd prefer keep it that way. > > http://cr.opensolaris.org/~mbp/pkinit/usr/src/lib/krb5/kadm5/clnt/client_rpc.c.wdiff.html > > 44 + memset((char *)&clnt_res, 0, sizeof(clnt_res)); > > * memset needs cast > (many in this file) See above comment about lint. > > http://cr.opensolaris.org/~mbp/pkinit/usr/src/lib/krb5/kadm5/srv/logger.c.wdiff.html > 1069 + strncpy(outbuf, ctime(&now) + 4, 15); > > * strncpy needs (void) cast > (also the sprintf calls too) As above. > > http://cr.opensolaris.org/~mbp/pkinit/usr/src/lib/krb5/kadm5/srv/svr_principal.c.wdiff.html > > 1136 + adb->old_keys = (osa_pw_hist_ent *) > 1137 + malloc((nkeys + 1) * sizeof > (osa_pw_hist_ent)); > > * malloc does not need cast > (below it memset does need cast on retval) As above. > > http://cr.opensolaris.org/~mbp/pkinit/usr/src/lib/krb5/kdb/kdb_cpw.c.wdiff.html > > * lots memcpy/memset need (void) ret cast As above. > > http://cr.opensolaris.org/~mbp/pkinit/usr/src/lib/krb5/plugins/kdb/db2/libdb2/hash/hash.c.wdiff.html > > > 114 + if (!(hashp = (HTAB *)calloc(1, sizeof(HTAB)))) > > * no cast needed for calloc ret As above. > > http://cr.opensolaris.org/~mbp/pkinit/usr/src/lib/krb5/plugins/kdb/db2/libdb2/mpool/mpool.c.wdiff.html > > 396 +#if defined(DEBUG) || defined(PURIFY) || 1 > > * || 1 ??? The comment from MIT's snv history reads: "Always zap newly allocated pages, intead of conditional on PURIFY. Minor performance penalty; c'est la vie. Better to be able to run purify or valgrind or whatever on the binaries we actually use." I guess we don't care about purify or valgrind? Is it worth the perf gain to remove this flag and maintain the diff with MIT? > > > > more later...glenn Thanks, -M
