Compiler warnings

[Ethan Galstad]

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:

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.

I should have caught this when I originally ran sed.  The my_free() 
macro def has been changed and 1200+ my_free() calls have been modified 
to work correctly again. Fix is in CVS. :-)


Ethan Galstad,
Nagios Developer
---
Email: nagios at nagios.org
Website: http://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/