Fix krb5-related debug #915
Conversation
|
Hi @alexey-tikhonov, thanks for the patches. I had a comment about the 3rd one, please see inline. bye, |
Ensure `sss_krb5_get_error_message()` never returns NULL as result is used in a lot of places where checks are not performed.
Few sss_krb5_get_error_message() related memory leaks were fixed. (Existing KRB5_DEBUG() macro did not fit to be used in those places)
alexey-tikhonov
force-pushed
the
fix_krb5_debug
branch
from
d3891d1
to
f1e4431
Compare
|
Hi @sumit-bose,
Thank you for the review. I think I have addressed your comment. |
src/providers/ldap/ldap_child.c
Outdated
| if (krberr) { | ||
| DEBUG(SSSDBG_OP_FAILURE, "Failed to get default realm name: %s\n", | ||
| sss_krb5_get_error_message(context, krberr)); | ||
| if (krberr != EOK) { |
There was a problem hiding this comment.
Hi,
thank you for adding back the messages. I now only have a minor nitpick. I prefer to use EOK only for our own functions where we know they have return EOK;. For library calls like e.g. here krb5_get_default_realm() I'd prefer with the documented return values, so 0 in this case. I'm sure there are place where we do not handle this consistently but since I noticed it here, I wanted to add this comment.
There was a problem hiding this comment.
Good catch. I will fix this.
Error handling was done wrong in a few aspects in ldap_child_get_tgt_sync() function. As per documentation: "The behavior of krb5_get_error_message() is only defined (1) the first time it is called after a failed call to a krb5 function using the same context, and (2) only when the error code passed in is the same as that returned by the krb5 function." Both (1) and (2) were violated heavily. Additionally in some cases ldap_child_get_tgt_sync() declared as returning `krb5_error_code` was actually returning non krb5 error code.
KEYTAB_CLEAN_NAME macro was replaced with `sss_printable_keytab_name()` function that provides real path in case of default keytab.
alexey-tikhonov
force-pushed
the
fix_krb5_debug
branch
from
f1e4431
to
9cebd16
Compare
|
Thanks, ACK. bye, |
This PR is continuation of cleanup work done in #883.
KEYTAB_CLEAN_NAMEmacro was replaced withsss_printable_keytab_name()function and a number of issues related with usage ofkrb5_get_error_message()were fixed.