On Tue, 27 Jan 2015, Petr Spacek wrote:
I don't know this piece of code so I can't say anything in particular.

Are you 100 % sure that it will not cause any harm in future? I'm hesitant to
introduce a goto branching which is not testable nowadays.

Personally I would prefer either assert() or moving the code as proposed in
the patch.
assert() (with exit) is wrong here. Logging a note -- OK.

I'm opposing moving the code because this is one of functions that gets
called for _every_ Kerberos authentication as we modify last
authentication time.

Does it matter? Does krb5_unparse_name() have an significant performance
impact? (I did not read the code so I'm asking.)
It is calling malloc()/realloc() so yes, it does matter. We cache
information about principal via DN in ied->entry_dn because we need the DN
instead of the principal in most cases except the case of adding a
principal to the database.

Given that the function is in authentication path, I think that it should fail
if original assumptions do not hold anymore.

I.e. I would modify the code to do this:
if (principal == NULL && entry->mask & KMASK_PRINCIPAL) {
   log("Sky is falling!");
   goto done;

This would require review if all code under done: label can handle all NULL
pointers gracefully.

... Isn't it too much work instead of one assert()? For an error condition
which cannot ever happen (you claimed)? :-)
What would assert do? abort() the process when compiled without NDEBUG?
I don't think it is what we need for the KDC.

there is also an unconditional dereference
ied = (struct ipadb_e_data *)entry->e_data;
without check that entry is not NULL. Maybe it can not happen in current code
but it should be supplemented with assert(entry != NULL) or other check.
Sometimes line has to be drawn somewhere. Whole MIT Kerberos is built
in this style where defence is happening in upper layers.

/ Alexander Bokovoy

Freeipa-devel mailing list

Reply via email to