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