Bug 93222 - Fix issues reported by Coverity
Summary: Fix issues reported by Coverity
Status: RESOLVED FIXED
Alias: None
Product: realmd
Classification: Unclassified
Component: adcli (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Stef Walter
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-12-03 10:43 UTC by Sumit Bose
Modified: 2015-12-07 09:28 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Coverity fixes (15.06 KB, patch)
2015-12-03 10:44 UTC, Sumit Bose
Details | Splinter Review
Coverity: silence a false positive (909 bytes, patch)
2015-12-03 10:45 UTC, Sumit Bose
Details | Splinter Review
Teach coverity about unexpected preconditions (983 bytes, patch)
2015-12-07 09:17 UTC, Stef Walter
Details | Splinter Review

Description Sumit Bose 2015-12-03 10:43:19 UTC
I did a Covertiy run with the latest git master and the attached patches fixes all reported issues.

Most of the issue were RESOURCE_LEAKs, i.e. memory leaks. One might argue that for a short running process like adcli fixing memory leaks is not that important. But having them around might make it harder to spot real issues in the output of Coverity or other code analysers. Additionally since most of them were found in library/* other projects might want to re-use some of the calls in future.

The first patch contains code fixes. At one point I added a goto to a cleanup section. If goto should be avoided in adcli I'd happy to change this but I think the code is more readable this way.

The second patch just has two Coverity annotations to suppress a false-positive warning (Coverity cannot know that the file descriptor is kept internally by libldap).
Comment 1 Sumit Bose 2015-12-03 10:44:34 UTC
Created attachment 120296 [details] [review]
Coverity fixes
Comment 2 Sumit Bose 2015-12-03 10:45:01 UTC
Created attachment 120297 [details] [review]
Coverity: silence a false positive
Comment 3 Stef Walter 2015-12-07 09:00:47 UTC
Attachment 120297 [details] pushed as 4d3804d - Coverity: silence a false positive

Merged the second patch.
Comment 4 Stef Walter 2015-12-07 09:15:22 UTC
(In reply to Sumit Bose from comment #0)
> I did a Covertiy run with the latest git master and the attached patches
> fixes all reported issues.
> 
> Most of the issue were RESOURCE_LEAKs, i.e. memory leaks. One might argue
> that for a short running process like adcli fixing memory leaks is not that
> important. But having them around might make it harder to spot real issues
> in the output of Coverity or other code analysers. Additionally since most
> of them were found in library/* other projects might want to re-use some of
> the calls in future.

adcli uses a pattern where we don't try to cleanup after unexpected failure. We complain loudly. This is the precondition coding style you see in many projects. For example, for most memory allocations to fail is considered unexpected. We don't dereference the null pointer, but print out diagnostics and return immediately.

Coverity needs to be taught to treat _adcli_precond_failed() so that it doesn't return. I'll post a patch for that.

If adcli is to bet used as a low level library in a project that has insists on treating unexpected errors as possible errors, then some of this will indeed need to change.

If given a good reason, I'm not against including code to change this, but such a change would need to be done properly, and it's not just about resource leaks. Such a change would remove almost all of the return_xxx_if_fail() macros.

I'll remove these cases from the failure for now, and commit a patch to teach Coverity about how adcli currently handles unexpected failures.

> The first patch contains code fixes. At one point I added a goto to a
> cleanup section. If goto should be avoided in adcli I'd happy to change this
> but I think the code is more readable this way.

goto for cleanup is fine.

> The second patch just has two Coverity annotations to suppress a
> false-positive warning (Coverity cannot know that the file descriptor is
> kept internally by libldap).

I've merged this.
Comment 5 Stef Walter 2015-12-07 09:17:48 UTC
Created attachment 120383 [details] [review]
Teach coverity about unexpected preconditions

adcli treats unexpected failures (such as most memory allocation failures)
differently from expected, possible failures. We don't do cleanup after
the former.

This patch tells Coverity that we don't expect these code paths to
have defined behavior.
Comment 6 Stef Walter 2015-12-07 09:28:48 UTC
Attachment 120296 [details] pushed as eadeb5b - Coverity fixes
Attachment 120383 [details] pushed as c489129 - Teach coverity about unexpected preconditions

Merged remaining changes, as described above. The 'Coverity fixes' patch
has only been partially merged.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.