Ironically, heimdal doesn't check for malloc failures for other dynamic libcrypto allocations here and in several other places.

Bah, this probably needs to free 'output_message_buffer'

This needs to drop the ctx_id_mutex and probably needs to memset 'deskey' and 'schedule' on failure. Some other places I moved the malloc up above the mutex, so I should probably do that here as well.

I'm not sure if this should memset deskey and schedule. The error return on line 143 for GSS_S_BAD_MIC doesn't clear them, but perhaps that is a bug?

Note that this GSS_S_BAD_MIC failure does clear deskey and schedule.

Here I chose to reuse the previously-allocated context to reduce the amount of error handling.

BN_set_negative() assumes 'bn' is not NULL.

This was already redundant in the existing code.

The old code just didn't check if 'n' and 'e' were NULL per the comment, but it doesn't seem like RSA can work without them. However, given the old code failing with an error rather than calling _hx509_abort() seemed more reasonable.

This function returns void so there wasn't an easy way to handle failure here more gracefully. Part of the issue is that the old code assumed that mallocing the 'key' object was the only thing that could fail, but now we can't assume a static size for the EVP contexts, so we end up with a struct containing pointers to two EVP contexts.

I changed this function to return an int to avoid just calling abort() if malloc failed. I'm not sure how public of an API this is such that changing it to an int is ok or if we need to resort to abort() instead. This is the only function whose API I changed in this fashion.

Having warnings enabled was really helpful, and I'm surprised we currently don't have them enabled at all for kerberos5. I haven't yet tested a build with GCC to see if the CWARNFLAGS need to be pan-compiler rather than just for clang.

Several places in kerberos use abs() on time_t values and clang warns about passing a long when time_t is 64-bits to abs() and wants to use labs() instead. I chose not to fix those and just suppress the warnings for now to avoid noise in the diff.

heimdal has krb5_get_err_text() marked as deprecated, but all of the tools use it still.

This appears to be a stub for older versions of OpenSSL? In 1.1 these are functions rather than macros, so the #ifndef didn't work properly to detect the existing version.

I tried to build with TARGET_ARCH=mips to test a GCC build but that failed because only the x86 openssl-conf.h.in has been updated.

Please try again. I have updated opensslconf.h for all architectures.

(But it's fixed now: https://github.com/heimdal/heimdal/pull/426#issuecomment-423759929)

Even on CHERI it is ok as loads and stores of pointers are still atomic and that's all you need for this to work.

Would you rather me change it to abort?

Yes, other places do that and I generally chose to match the existing style of whatever the current code used (either one check at the end or individual checks). It probably would be cleaner as you suggest though.

I went ahead and used heim_ntlm_free_buf() since it is already used elsewhere in this file.

It's fine with EINVAL, so I'd leave it alone for now.

One could perhaps argue that we should zero out &deskey before returning, though there's already a path a few lines later that returns without clearing it.

I guess we could hx509_set_error_string() here if we wanted.
Also it looks like we leak clear.data.

This leaks data.data; need to krb5_data_free(&data)

heim_ntlm_free_buf(infotarget)

I think this should actually be clearing 'deskey' and not schedule in which case that fixes the issue you noted about a missing clear?

I chose to reuse the 'out' label similar to the error case a few lines above.