change_password resource cleanup?

[Greg Hudson]

On 6/28/25 00:50, Joonas Tuomisto wrote:
> krb5_change_password() returns the result_code_string and result_string
> krb5_data structures. Are you supposed to free() them - or I suppose more
> specifically, krb5_free_data() them after use?

They should be freed with krb5_free_data_contents() when the caller is 
done with them.

krb5_free_data() would free the krb5_data pointer itself, which would be 
asymmetric (the pointer is supplied by the caller and not allocated by 
krb5_change_password()) and often a memory violation.

> The documentation for krb5_chpw_message() explicitly calls out that you
> should krb5_free_string() its message_out. For krb5_change_password() it
> does not.

We usually document freeing requirements for libkrb5 function outputs, 
and it is indeed missing for krb5_change_password().  I have made a note 
to fix this.  (Also missing: result_code_string may be null, although 
result_string is required.)

> Are these krb5_free_* functions always safe to call or should you only
> assume the structs exist (and require cleanup) when e.g.
> krb5_change_password() returns with result_code set?

There are early exit paths from krb5_change_password() that do not set 
*result_code_string or *result_string.  This is not really ideal; we 
generally try to initialize output arguments in library APIs, but a lot 
of libkrb5 was written before that became a common practice.  I have 
made a note to fix this.

(libgssapi_krb5 is much, much better about initializing output arguments 
along all exit paths.)

krb5_free_*() functions are okay to call with null pointers, or with 
structures containing null pointers (e.g. krb5_free_data_contents() on a 
krb5_data structure whose data element is null).

> My code now has if (retval) ... if (result_code) ... sprinkled and I can
> already see myself how you could easily end up with resource leaks with C
> and different code paths. A common cleanup path helps.

In spite of krb5_change_password() not always setting output arguments, 
the following should be safe:

     krb5_error_code ret;
     krb5_data result_code_string = { 0 }, result_string = { 0 };

     ...
     ret = krb5_change_password(context, ..., &result_code_string, 
&result_string);
     if (ret)
         goto cleanup;
     ...

cleanup:
     krb5_free_data_contents(context, &result_code_string);
     krb5_free_data_contents(context, &result_string);