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