On Fri, 2008-09-19 at 10:26 -0400, Peter Shoults wrote: > Mark Phalan wrote: > > I've just uploaded a webrev of my resync/pkinit workspace. There still > > needs to be some work on pkinit so don't expect the code in > > usr/src/lib/krb5/plugins/preauth/pkinit/ to be complete (you can ignore > > it for now). I'll post another incremental webrev with any changes I > > make to the pkinit code later on. The rest of the changes are resync > > changes for MIT 1.6.3. The hg comment needs to be updated, I'll do that > > once we get the pkinit PSARC case submitted. > > > > I've chunked the review up into four pieces as I expect the krb team to > > do the review. > > > > Shawn: Chunk 1 > > Peter: Chunk 2 > > Glenn: Chunk 3 > > Will: Chunk 4 > > > > I'd like to have this completed by 17th Sept. Let me know if thats a > > problem for anyone. > > > > webrev here: > > http://cr.opensolaris.org/~mbp/pkinit/ > > > > Cheers, > > > > -Mark > > > > > > > > > > > > > > _______________________________________________ > > kerberos-discuss mailing list > > kerberos-discuss at opensolaris.org > > http://mail.opensolaris.org/mailman/listinfo/kerberos-discuss > > > > Couple days late, but finally got thru my chunk. Here are my comments: > > usr/src/lib/gss_mechs/mech_krb5/krb5/krb/auth_con.c > > Still has Sun Copyright. I assume because of the > few /* Solaris Kerberos */ comments. If so, then > filea: > > usr/src/lib/gss_mechs/mech_krb5/krb5/keytab/read_servi.c > usr/src/lib/gss_mechs/mech_krb5/krb5/krb/chpw.c > > should not have had the copyright removed.
The differences between the Solaris versions of read_servi.c and chpw.c are not large enough to warrent a copyright notice any more (no change to object code). Part of the changes to auth_con.c were to allow it to be compiled as part of the kernel mech - a change which could plausibly add new copyright. > > ------------- > > Conversely, why is the copyright kept in the following > files: > > usr/src/lib/gss_mechs/mech_krb5/krb5/krb/conv_princ.c > It probably shouldn't be. I'll mail our lawyers and make sure that they're happy that I remove any copyright notices. > --------------------- > > usr/src/lib/gss_mechs/mech_krb5/krb5/krb/chk_trans.c > > What is that on line 360 of your modified code? CTRL-l. Linefeed (I think). > > If the user passes a -x and/or a -v, that would populate > argv[1] and argv[2] respectfully, right? If so, then I > am not sure the subsequent references to argv[1] and > argv[2] - like lines 395 thru 398. It's a moot point. We don't compile that code into our mech (see the preceding '#ifdef TEST'. > > ------------------ > > usr/src/lib/gss_mechs/mech_krb5/krb5/krb/chpw.c > > If we return at line 115, where does the free that > is on line 119 take place? Memory leak maybe? Nope. krberror won't have been allocated as krb5_rd_error() will have failed if we return at line 115. > Actually, > it looks like a lot of the returns in this routine > do not call krb5_free_error(context,krberror) and also > krb5_xfree(clearresult.data). It looks mostly correct to me. I believe I see a possible mem-leak. Fixed. > > -------------- > > usr/src/lib/gss_mechs/mech_krb5/krb5/krb/copy_addrs.c > > Should there be a /* Solaris kerberos */ comment > above line 45 and possibly a Sun Copyright? The only difference I see between MIT's copy_addrs.c and our copy_addrs.c is on line 32 - and its just a lint comment. Can you elaborate? > > ------------ > > usr/src/lib/gss_mechs/mech_krb5/krb5/krb/copy_data.c > > ditto on above comment for lines 55 and 75 and copyright? > Ditto on the above comment for copy_data.c > --------------- > > usr/src/lib/gss_mechs/mech_krb5/krb5/krb/gic_pwd.c > > 357 - I do not see where result string is set to any value > before you could attempt to free it at lines 368 and/or 377 Yeah, thats weird. I've ifdef'ed out the result_string related lines. > Looks like kadm5_chpass_principal_util is supposed to be > call to krb5_change_password. Why? > > --------------- > > usr/src/lib/gss_mechs/mech_krb5/krb5/krb/krb5_libinit.c > > Did you mean to change the modes on this file? > > mode change: 755 to 644 Yes. I see no reason why it should be executable. > > -------------- > > usr/src/lib/gss_mechs/mech_krb5/krb5/krb/preauth.c > > I did not review this file as I assume it is a direct > copy of MIT stuff....did you want me to review this logic? > Yup :) > ---------- > > usr/src/lib/gss_mechs/mech_krb5/krb5/os/changepw.c > > It appears that they just moved code around a bunch, and > added some new things. I did not see any issues with > the logic, but would think this this code path would be > hit a lot and would be quite noticable if something is > not right. Testing this did not run into issues, right? I didn't see any failures when I ran the standard Kerberos STC1/2 test suites. > > ------------ > > usr/src/lib/gss_mechs/mech_krb5/krb5/os/locate_kdc.c > > is line 698 suppose to be "case kdc_ports:"? Nope. kdc_ports is a label here. Note the "goto" further down on line 704. > > -------------- > > usr/src/lib/gss_mechs/mech_krb5/krb5/os/sendto_kdc.c > > 68 #undef DEBUG > 69 > 70 #ifdef DEBUG > > How can 70 ever be true if you #undef it at line 68? It can only be true if the developer decides to manually change the code and re-compile. We don't want DEBUG bits in our builds so this shouldn't be enabled unless we're trying to debug something. Thanks for the review. -Mark
