[SSSD] [PATCH] Remove useless assignment to function parameter

Lukas Slebodnik lslebodn at redhat.com
Wed Feb 26 19:18:49 UTC 2014


On (26/02/14 13:46), Simo Sorce wrote:
>On Wed, 2014-02-26 at 19:36 +0100, Lukas Slebodnik wrote:
>> On (26/02/14 13:00), Simo Sorce wrote:
>> >On Wed, 2014-02-26 at 18:33 +0100, Lukas Slebodnik wrote:
>> >> ehlo,
>> >> 
>> >> Reported by: cppcheck
>> >> 
>> >> void free_fun(struct info *info)
>> >>      free(info->name);
>> >>      free(info);
>> >>      info = NULL;
>> >>      ^^^^^^^^^^^
>> >> Assignment to function parameter has no effect outside the function.
>> >> 
>> >> Patch is attached.
>> >
>> >Although the assignment is useless I do not like the solution.
>> >
>> >I think the solution is not to remove the assignment but to change the
>> >function prototype to take a pointer to the pointer we want to free so
>> >that at the end of the function we can assign NULL to the original
>> >variable we have been called to free.
>> >Otherwise the caller has a dangling pointer.
>> >
>> >So the solution for me is:
>> >void free_fun(struct info **_info) {
>> >    struct info *info = *_info;
>> >    free(info->name);
>> >    free(info);
>> >    *info = NULL;
>> >}
>> >
>> >Simo.
>> >
>> I do not agree with you.
>> 
>> Free functions have parameter (void *) which will be freed.
>> e.g.:
>>     void free(void *ptr);
>>     int talloc_free (void *ptr)
>>     /**
>>      * Free a string allocated by a krb5 function.
>>      *
>>      * @param [in] context          Library context
>>      * @param [in] val              String to be freed
>>      *
>>      * @version First introduced in 1.10
>>      */
>>     void KRB5_CALLCONV
>>     krb5_free_string(krb5_context context, char *val);
>> 
>> If you do not want to have dangling pointer, you need to assign NULL yourself
>> or use macro.
>
>gssapi OTOH uses th convention that you have to pass a pointer and then
>NULLs the one you passed int, and I find it much less prone to mistakes.
>
>> #define talloc_zfree(ptr) \
>>     do { talloc_free(discard_const(ptr)); ptr = NULL; } while(0)
>
>Note that it is common agreement even upstream (I think) that this is an
>unfortunate side-effect of not having thought about doing this right
>beofre the API was finalized.
>
>Having to define macros is not a good idea in general, it adds
>unnecessary churn.
>
It is a workaroud if you cannot change API and don't want to make mistakes.

>> I do not want to introduce new "semantic" to free functions.
>
>They are not new in any way, a lot of APIs use the semantic I recommend
>and it is a better one, in general.
>
I like this idea.

>> In many cases, you cannot change API of free function, because they are used
>> as a callback.
>
>If there is a technical issue that prevents changing a specific function
>then that's fine, but I prefer the way I shown as a general solution, I
>started using it quite a while ago in all projects I am active in.
>
1. sssd_krb5_locator_close is callback in krb5plugin_service_locate_ftable
    krb5 locator plugin

2. free_exp_data is callback used by function pam_set_data

Only function hbac_free_info can be changed; but it will be change of API
in ipa/ipa_hbac.h. In this case, I would prefer to file a ticket
and to do similar change also in libsss_idmap library
    {sss_idmap_free, sss_idmap_free_sid, sss_idmap_free_dom_sid,
    sss_idmap_free_smb_sid, sss_idmap_free_bin_sid}

LS



More information about the sssd-devel mailing list