ghudson at MIT.EDU
Fri Sep 11 00:27:03 EDT 2009
Here are my comments from my (hopefully) final review of the S4U code.
Please address the following and then merge with the latest rev of the
* add_pa_data_element appears to leave out_padata in an invalid state
when the if (copy) block fails to allocate memory. While you are there,
please rename "out_padata" since it is an in/out parameter, not an
* kvno should check for -P without -k after parsing all of the
options, so that option order is not important.
* kvno.c:226 should use &server instead of &in_creds.server, I believe
(that is, you missed a case when altering the logic).
* The new kvno options should be documented in the man page.
* s4u_gss.glue.c:kg_get_evidence_ticket is unused; it should be
removed unless there's a reason for it that I couldn't see.
* The kfree.c changes to krb5_free_checksum_contents and
krb5_free_unparsed_name are unnecessary and undesirable; please revert
them (or explain them if I am misunderstanding).
I have some other notes, which I am not going to require changes for:
* I wasn't sure what the get_in_tkt.c changes were for. They relate
to changing the server name for referrals in an as-req, which seems only
peripherally related to S4U at most.
* I wasn't able to intuit what "gcvt" was supposed to stand for, even
after you added the comment to krb5int_send_tgs.
* There's some mech logic in s4u_gss_glue.c (conversion between OID
membership in a list and prerfc_mech/rfc_mech flags) which appears to be
common with acquire_cred.c and should be factored out.
* Going forward, the preferred pattern for handling output parameters
is to (1) initialize them to NULL (or something appropriate for
non-pointer types), and (2) not touch them after that until a successful
return is guaranteed. This facilitates static analysis tools like
Coverity in determining the correspondence between return values and
allocation of return pointers.
* In future work, please briefly document the purpose of new internal
functions with a comment above the function definition.
* I got slightly lost in the mechglue changes, so I wasn't able to
review those as carefully as the rest of the code.
More information about the krbdev