[systemd-devel] [PATCH] STACK (http://css.csail.mit.edu/stack/) found issues for unstable optimized code again.

[Lennart Poettering]

On Fri, 07.02.14 16:39, Stefan Beller (stefanbeller at googlemail.com) wrote:

> The first problem arises in src/gudev/gudevdevice.c
> In lines 688 and 882 we call the function split_at_whitespace,
> which is just a wrapper around g_strsplit_set, but removes also
> the empty strings.
> 
> Now in this wrapper function we have a 'gchar **result;' on
> which we'll operate. In line 638 we start on removing
> the empty strings by checking them one by one.
> Before the for loop we could mentally add an
> 	assert(result);
> because if this assertion doesn't hold, the access
> of result[n] would segfault.
> 
> Now compilers, which optimize heavily such as gcc 4.8,
> will take notes and remember this implicit assertion in
> case it can optimize something later on based on
> this assumption.
> 
> And indeed it can do so, when returning from the function
> split_at_whitespace in lines 688 and 882. There are checks
> for this assertion being violated:
>   if (result == NULL)
>     goto out;
> But the assertion must hold, or we'd have been segfaulting
> earlier. That means either, we can remove the two lines
>   if (result == NULL)
>     goto out;
> or we forgot to check the results of g_strsplit_set properly.
> 
> Now it's time to read the documentation on that
> https://developer.gnome.org/glib/stable/glib-String-Utility-Functions.html#g-strsplit-set
> And as far as I understand that, g_strsplit_set will never return a plain
> NULL, but at least a pointer to a NULL. So the implementation of the
> wrapper seems alright and we can remove the superfluous lines checking
> for NULL.

Not following. Your patch looks like an optimization, not a bugfix. But
your mail subject line suggests there was something broken. Can you
elaborate? I don't follow...

Lennart

-- 
Lennart Poettering, Red Hat