Page MenuHomeFreeBSD

Fix the file-descriptor resource leakage as reported by Coverty
ClosedPublic

Authored by aniketp on Jun 12 2018, 8:00 PM.

Details

Summary

Coverty reported leakage in resources in some of the audit tests which involved opening a
file and not clcosing it. This revision fixes that issue.

Also, some more ATF_REQUIRE statements to restrict errors.

NOTE: Earlier version had this snippet
utils.c:199 close(fd[0].fd);

returns -1 since the pipestream has already been closed one line above. So removed it

Test Plan

A part of report can be found here.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

aniketp created this revision.Jun 12 2018, 8:00 PM
aniketp edited the summary of this revision. (Show Details)Jun 12 2018, 8:01 PM
aniketp set the repository for this revision to rS FreeBSD src repository.
asomers requested changes to this revision.Jun 12 2018, 8:55 PM
asomers added inline comments.
tests/sys/audit/file-attribute-access.c
63 ↗(On Diff #43669)

A comment that simply repeats what the code does isn't a useful comment (except in assembly programming, where the code is filled with stuff like VPCLMULQDQ). You should delete these comments, or replace them with something like "teardown".

155 ↗(On Diff #43669)

It's usually a bad idea to put assertions in teardown code, because a failed assertion can prevent the rest of the teardown from running. In this case it doesn't matter, because there's only one teardown statement. Still, it would be conventional to omit the ATF_REQUIRE_EQ.

tests/sys/audit/open.c
63 ↗(On Diff #43669)

Mixing whitespace changes with functional changes is hard to review. Could you please revert the whitespace changes for now?

This revision now requires changes to proceed.Jun 12 2018, 8:55 PM
aniketp updated this revision to Diff 43686.Jun 13 2018, 3:26 AM

Remove ATF_REQUIRE_EQ assertion from close() statements (and the comments)

aniketp marked 2 inline comments as done.Jun 13 2018, 3:32 AM
aniketp added inline comments.
tests/sys/audit/open.c
79 ↗(On Diff #43686)

@asomers , formatting the backslashes to be at the limit of 80 character line (which looks better) took quite a bit of hard work. Undoing them would be really cumbersome. Can you please review in the current state?

I've only added these lines in open.c

55: static int filedesc;   [Global]
79: close(filedesc);
119: close(filedesc);
120: close(filedesc2);

And modified the respective open() statements to store the return value in filedesc.
Rest of the code was already reviewed by you in D15657.

asomers added inline comments.Jun 13 2018, 3:44 AM
tests/sys/audit/open.c
79 ↗(On Diff #43686)

Could you revert everything else then and submit a style-only change? It's not just about ease-of-review. It's also important for future programmers who try to figure out what this change did and why it did it. And yourself is included among those future programmers; a year from now you won't be able to remember what you did.

aniketp updated this revision to Diff 43690.Jun 13 2018, 5:20 AM

Remove backslash formatting from open(2)'s test program macro
[Unrelated to this revision]

  • Also add some ATF_REQUIRE assertions in utils.c. As their successful execution is important
  • Correct includes' order in utils.c
asomers accepted this revision.Jun 13 2018, 3:14 PM
asomers added inline comments.
tests/sys/audit/open.c
119 ↗(On Diff #43690)

It doesn't matter in this case, but in general it's a good idea to release resources in the LIFO order as opposed to FIFO order. That way you never have an invalid combination of resources open.

This revision is now accepted and ready to land.Jun 13 2018, 3:14 PM
aniketp updated this revision to Diff 43710.Jun 13 2018, 3:31 PM

Invert the closing of file descriptors for open and openat following LIFO rule

This revision now requires review to proceed.Jun 13 2018, 3:31 PM
asomers accepted this revision.Jun 13 2018, 4:57 PM
This revision is now accepted and ready to land.Jun 13 2018, 4:57 PM
This revision was automatically updated to reflect the committed changes.