Ethan Galstad wrote:
> Andreas Ericsson wrote:
>> Andreas Ericsson wrote:
>>> Andreas Ericsson wrote:
>>>> Ethan Galstad wrote:
>>>>> Andreas Ericsson wrote:
>>>>>> Ahoy.
>>>>>>
>>>>>> I was digging around trying to fix a bunch of compiler-warnings, and
>>>>>> noticed that pretty much every invocation of my_free() resulted in
>>>>>> about a million warnings, such as this:
>>>>>>
>>>>>> ../xdata/xrddefault.c:190: warning: dereferencing type-punned
>>>>>> pointer will break strict-aliasing rules
>>>>>>
>>>>>>
>>>>>> Considering that not a single invocation of my_free() evaluates the
>>>>>> return code of the function, a macro such as
>>>>>>
>>>>>> #define my_free(ptr) { if(ptr) { free(ptr); ptr = NULL; } }
>>>>>>
>>>>>> would do the trick, and also provide a slight performance
>>>>>> improvement.
>>>>>>
>>>>>> Since a patch would be fairly huge, and I've kinda filled my quota
>>>>>> for huge patches for today, the following sed-script will take care
>>>>>> of the call-sites (requires sed 4.0.9 or later):
>>>>>>
>>>>>> sed -i 's/my_free[^&]*&\([^)]*\).*/my_free(\1);/' */*.c
>>>>>>
>>>>> Definitely a good idea. I've committed this to CVS after some
>>>>> manual massaging post-sed.
>>>>>
>>>> Thanks.
>>>>
>>>>> Don't know why, but I get a SIGABRT if I don't comment out two
>>>>> my_free() statements in xdata/xodtemplate.c in the
>>>>> xodtemplate_get_inherited_string() function.
>>>>>
>>>> I'll look into it.
>>>>
>>>
>>> Hmm. I can't reproduce it, as I don't have a decent config with template
>>> inheritance using + concat stuff. Have you got anything readily made to
>>> share? I'll hack some up otherwise, but I'm not sure I'd get one that
>>> could trigger it.
>>>
>>> I'm fairly certain what's happening though. The caller passes the string
>>> by reference, but we're free()'ing it as if it wasn't, so we're
>>> basically
>>> calling free() on a pointer to the caller's stack frame. Bad thing.
>>> Change them from
>>>
>>> my_free(this_value);
>>>
>>> to
>>>
>>> my_free(*this_value);
>>>
>>> and things might turn out ok. Otherwise, tar up your config and send me
>>> what you've got and I'll see what I can do.
>>>
>>
>> Got the config, verified the crash, and here's the patch.
>>
> [snip]
>
> Actually, the real fix is much bigger. The my_free() function should
> always be passed a pointer to a pointer. Example:
>
Except that it's not a function anymore, so it doesn't need to get a
pointer to the pointer, only the pointer itself.
> char *temp_buf=strdup("Something");
> my_free(&temp_buf);
> /* temp_buf has been free()'d and is now NULL */
>
> The double pointer reference allows my_free() to set the original
> pointer to NULL. If this isn't done, the NULL assignment doesn't work
> as expected.
>
Not quite. That's true when you're calling it as a function, but when
it's a macro, *not* passing the pointer by reference is the right
thing to do.
What you've got in the macro now:
#define my_free(ptr) { if(ptr && *ptr) { free(*ptr); *ptr = NULL; } }
will cause subtle errors when you do
char *buf = malloc(x);
my_free(buf); /* forgot to "pass" by reference */
In this case, buf is only free()'d if its first char isn't nul, but
the compiler won't complain.
The other error-case where the compiler wouldn't complain was a lot
more unusual: A function gets a parameter passed by reference and
forgets to "pass" it to my_free() the way it uses it to access the
memory area it points to, like so:
char *buf = malloc(x);
foo_func(&buf);
foo_func(char **buf)
{
printf("%s", buf); /* fails, should be '*buf' !! */
my_free(buf); /* should be '*buf' !! */
}
Either way, the code in the macro right now is near-identical to the
original, but slightly slower as it has one more condition to evaluate,
although that'll only be an "or ptr,*ptr" instead of "or ptr,ptr" which
is only an issue on registry starved systems. Unfortunately, x86 systems
fall into that category.
--
Andreas Ericsson [email protected]
OP5 AB www.op5.se
Tel: +46 8-230225 Fax: +46 8-23023
...[email truncated]...
This post was automatically imported from historical nagios-devel mailing list archives
Original poster: [email protected]