Created attachment 281418 [details] [review]
goption: Add g_free() in error path

Pacifies static analysis.

Created attachment 281419 [details] [review]
gfile: Initialize variable to pacify static analysis

Not a real bug, but will quiet the analysis.

Created attachment 281420 [details] [review]
gfileutils: Add missing g_free() in error path

Discovered by static analysis.

Created attachment 281421 [details] [review]
gtlsinteraction: Make precondition in middle of function just be assertion

We're using a precondition in the middle of the function, and if we
hit it, we leak the closure.

Change this to just be an assertion.  Anyone who was attempting to use
this API would have quickly noticed it didn't work.

Created attachment 281422 [details] [review]
gthreadedresolver: Unref unexpected address

I don't believe any real app would hit this, but we do leak
if it occurs.

Spotted by static analysis.

Created attachment 281423 [details] [review]
gsocketclient: Unref object in error path

Another unlikely one, but let's pacify static analysis and clean up.

Created attachment 281424 [details] [review]
gsettingsschema: Close directory

Spotted by static analysis.

Created attachment 281425 [details] [review]
compile-resources: Unref objects in error path

Here, you see, we *really* want __attribute__((cleanup))...

Created attachment 281426 [details] [review]
gapplicationimpl-dbus: Fix leak of fd list object

Spotted by static analysis.

Created attachment 281427 [details] [review]
gbacktrace: If dup2 happened to return -1, don't call dup2 with same value

Shouldn't happen, but let's pacify static analysis.

Created attachment 281428 [details] [review]
gsettingsschema: Suppress return value check

We're intentionally ignoring the value here.  Pacifies static
analysis.

Created attachment 281429 [details] [review]
glocalfileinfo: Suppress static analysis return value warning

Just ignore the return value, since we're checking contents != NULL.

Created attachment 281430 [details] [review]
gkeyfilesettingsbackend: Add assertion for known-true condition

Should pacify static analysis.

Created attachment 281431 [details] [review]
gcredentials: Add assertion to pacify static analysis

This should always be true.

Created attachment 281432 [details] [review]
gapplicationimpl-dbus: Check return value of g_variant_iter_next

Pacifies static analysis.

Created attachment 281433 [details] [review]
gobject: Tweak conditional to pacify static analysis

It can't easily see that value is always non-NULL here; this
equivalent tweak will show that it is.

Created attachment 281434 [details] [review]
gnetworkmonitornetlink: Add NULL check

I think this should never be hit, so we're just pacifying static
analysis.

Review of attachment 281418 [details] [review]:

I'd much rather we just drop this 'sanity check' entirely.  It's completely redundant.

If anything, it could be replaced with an assert.

Review of attachment 281419 [details] [review]:

OK.

Review of attachment 281420 [details] [review]:

Nice.

Review of attachment 281421 [details] [review]:

I think there may have been an intent that we do this in order not to crash...

Are you trying to avoid errors about properly freeing things due to the (effective) return here?

Review of attachment 281422 [details] [review]:

How about

if (!IS (...))
  g_clear_object ();
  continue;

as a more concise way to say the same?

Only a suggestion; if you think that your way is better for being clearer then I'm also OK with that (and you should commit it now).

Review of attachment 281423 [details] [review]:

This function is gross -- too much if/else interaction going on here...

Any reason we can't move the check for IS_TCP_CONNECTION above the proxy creation?

Also: while here it might be nice to take the 'else if'/else under the if(proxy) and turn it into a

else
  {
    if ()
      {
      }
    else
      {
      }
  }

since logically this is closer to what actually goes on there....

Review of attachment 281424 [details] [review]:

I make this mistake more often than I care to admit...  thanks :)

Review of attachment 281423 [details] [review]:

This function is gross -- too much if/else interaction going on here...

Any reason we can't move the check for IS_TCP_CONNECTION above the proxy creation?

Also: while here it might be nice to take the 'else if'/else under the if(proxy) and turn it into a

else
  {
    if ()
      {
      }
    else
      {
      }
  }

since logically this is closer to what actually goes on there....

Review of attachment 281425 [details] [review]:

This is also a bit ugly -- but it would be a bit of work to do this any better, I think...

btw: we seem to leak 'data' in (every) error case here....  or did I miss something?

Review of attachment 281426 [details] [review]:

Unlikely to cause problems since this process will exit immediately anyway, but clearly completely correct.  Thanks :)

Review of attachment 281427 [details] [review]:

This kind of patch upsets me a bit -- just enough to make static analysis quiet without actually fixing the potential problem here.

As unlikely as this is, it represents a potentially serious problem and we should report it when we discover it: immediately after the failed dup() call, before we close the original stderr and have lost it forever.  If we can't do the dup() then let's just report that error immediately and exit.

Review of attachment 281428 [details] [review]:

Can you add a comment just above (as I should have myself) that this is "best effort only" and that it is our intent to silently ignore errors?

Review of attachment 281429 [details] [review]:

OK....

Review of attachment 281430 [details] [review]:

In general, and specifically here, I'd prefer if the commit messages contained a bit more about the reasoning why this change is sane.

In this case, for example: "We are iterating the list of groups just returned by GKeyFile, so we can be certain that the group will exist."

Otherwise OK.

Review of attachment 281431 [details] [review]:

Good.

Attachment 281419 [details] pushed as feec280 - gfile: Initialize variable to pacify static analysis
Attachment 281420 [details] pushed as 49a5d0f - gfileutils: Add missing g_free() in error path
Attachment 281422 [details] pushed as e55a953 - gthreadedresolver: Unref unexpected address
Attachment 281424 [details] pushed as 3fb44c4 - gsettingsschema: Close directory
Attachment 281426 [details] pushed as acdcf5c - gapplicationimpl-dbus: Fix leak of fd list object
Attachment 281428 [details] pushed as 5fe71c7 - gsettingsschema: Suppress return value check
Attachment 281429 [details] pushed as 7ea4bf3 - glocalfileinfo: Suppress static analysis return value warning
Attachment 281430 [details] pushed as 8ebd287 - gkeyfilesettingsbackend: Add assertion for known-true condition
Attachment 281431 [details] pushed as 9182197 - gcredentials: Add assertion to pacify static analysis

Review of attachment 281421 [details] [review]:

Yeah, the warning is about leaking closure (see the first part of my commit message).

Created attachment 281470 [details] [review]
goption: Replace precondition with assertion

A static analysis run noted that we weren't freeing the cmdline in the
error path here.  We can just make this an assertion instead; I just
checked the kernel code, and it just usees a seq_printf() here which
will NUL terminate.

Created attachment 281473 [details] [review]
gtlsinteraction: Hoist precondition before allocation

We're using a precondition in the middle of the function, and if we
hit it, we leak the closure.

Let's allocate the closure per path; this allows us to allocate it
before path-specific preconditions, and better avoids a pointless
malloc/free pair in the unhandled case.

Review of attachment 281470 [details] [review]:

It's g_file_get_contents() that gives us the nul...

I think even having the assert here is pointless, but if you prefer to commit this (with a better message) then that's OK too.

Review of attachment 281473 [details] [review]:

This is nicer.  Thanks.

Review of attachment 281433 [details] [review]:

I almost made a similar change here myself recently.

Can you also remove the "to silence gcc" comments found on the NULL initialisations just above each of these patch fragments (since we now depend on NULL being there)?

Review of attachment 281432 [details] [review]:

seriously? :(

I noticed another case of this earlier -- with ignoring the return value of g_file_get_contents() because we were checking for the NULL on the returned 'contents' (in the hidden files code).

It seems like we have a certain common idiom in our code that the tool is having trouble with.  I wonder if maybe instead of trying to fix all of these cases, we could maybe just improve the tool...  The idea that we have functions that only set the 'out' parameters when they return TRUE (and only ever set it to non-%NULL) shouldn't be too hard to explain...

Comment on attachment 281473 [details] [review]
gtlsinteraction: Hoist precondition before allocation

Attachment 281473 [details] pushed as 7009e31 - gtlsinteraction: Hoist precondition before allocation

*** Bug 731304 has been marked as a duplicate of this bug. ***

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/905.