Created attachment 227918 [details] [review]
proposed fix

I don't know about these. I'm tempted to WONTFIX these.

We never care about memory leaks as a result of g_return_*() - those are just sanity checks to help spot programming bugs during development, rather than crashing right away.

But even if not, functions that take ownership of arguments should do so at *all* times, whether they then return an error or not, so it should never be the caller freeing stuff after giving away ownership, that's just wrong calling conventions IMHO.

However, I see that this is handled like this elsewhere in the code.. (e.g. gst_rtsp_message_add_header).

So in a way my objection to this patch is that it perpetuates this madness even more ;)

Setting to NEEDINFO, to see what other developers (and the reporter) think.

I agree with Tim if a function says it takes ownership of a value should do so always.

Thinking about this corner case it can be fixed this way. It's a bit ugly but it works. If you agree I can make a patch with this fix.

#include <glib.h>

void
main()
{
  gchar *x = g_strdup ("eserse");
  g_return_if_fail (NULL != NULL || (
          {
            g_free (x);
            FALSE;
          }
      ));
}

> Thinking about this corner case it can be fixed this way. It's a bit ugly but
> it works. If you agree I can make a patch with this fix.
> 
> #include <glib.h>
> 
> void
> main()
> {
>   gchar *x = g_strdup ("eserse");
>   g_return_if_fail (NULL != NULL || (
>           {
>             g_free (x);
>             FALSE;
>           }
>       ));
> }


Yeah, let's not do that. I think this is using a gcc and/or c99 extension anyway, but it could probably be done differently as well; but still, let's not.

I would suggest you either teach coverity or whatever tool you're using about g_return_*(), or make it process things with -DG_DISABLE_ASSERTS (or whatever the right define is).