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) {
krberr = KRB5_KDB_INTERNAL_ERROR;
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.
BTW
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
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
