Compiler warnings
Ethan Galstad
nagios at nagios.org
Mon Oct 22 15:33:21 CEST 2007
Andreas Ericsson wrote:
> 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.
>
Aha - you are correct! I ran some tests and not passing the buffer be
reference worked okay. I'll update CVS shortly...
Ethan Galstad
Nagios Developer
___
Email: nagios at nagios.org
Web: www.nagios.org
-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
More information about the Developers
mailing list