Fix memory leaks in pr(1)

[asomers]

Summary

Fix memory leaks in pr(1)

Also, hook NetBSD's pr test into the build, and add three more test cases.

Reported by: Coverity, Valgrind
CID: 271650 271651 271652 271653 271654 271655 271656 271656
CID: 271657 271658 271659 1006939 1006940 1006941 1006942 1009098

[ngie]

My concern with the change isn't the tests, but the inconsistent way it uses goto's and return values. Otherwise -- all good :)!

usr.bin/pr/pr.c

663–674 ↗

(On Diff #23883)

if (x)
    free(x);

can be reduced to:

free(x);

818 ↗

(On Diff #23883)

Why set retval if you're going to blindly return 0/1?

1038–1039 ↗

(On Diff #23883)

-> free(buf);

1792 ↗

(On Diff #23883)

What about other cases like line 1697/1761?

[ngie]

In D9137#190330, @ngie wrote:

My concern with the change isn't the tests, but the inconsistent way it uses goto's and return values. Otherwise -- all good :)!

Also, you're fixing some file descriptor leaks too, not just memory leaks..

[asomers]

usr.bin/pr/pr.c

818 ↗

(On Diff #23883)

Actually, that function doesn't set retval. It seemed like it would require fewer lines of code to use separate return statements in this function, but it would require fewer lines of code to use a single return statement with a variable argument in vertcol().

1792 ↗

(On Diff #23883)

Coverity didn't pick up on other cases. Odd. But on reexamination, I see that the specification for egetopt doesn't allow eoptarg to be NULL here (it does allow it at line 1761). I should probably just delete this chunk and mark the CID as a false positive.

[asomers]

Simplify usage of free(3)

[ngie]

usr.bin/pr/pr.c

356 ↗

(On Diff #24138)

Should retval default to 1 and be set to 0 if it makes it all the way to the bottom (to reduce unnecessary lines of code)?

848 ↗

(On Diff #24138)

Same here. Why not add retval, default it to 1, then set it to 0 at the bottom (thus reducing lines of change/duplicate code between the err goto and the non-err goto return path)?

[asomers]

Consolidate some error code to reduce line counts

[ngie]

Could you please update your tree (I'm seeing unnecessary diffs in etc/mtree/BSD.tests.dist)?

I'm going to [tentatively] accept the revision (the code changes to pr(1) look great!), because the items I'm noting are minor and are easy to fix pre-commit.

usr.bin/pr/tests/basic2.sh

2 ↗

(On Diff #26796)

chmod -x this file and the warnings will go away.

Also: these should be named "foo_test.sh", not "foo.sh".

[asomers]

I'll change basic2.sh to basic2_test.sh and fix its mode. But basic.sh comes from netbsd-tests, and I don't want to introduce unnecessary diffs by renaming it.

[ngie]

In D9137#212532, @asomers wrote:

I'll change basic2.sh to basic2_test.sh and fix its mode. But basic.sh comes from netbsd-tests, and I don't want to introduce unnecessary diffs by renaming it.

Yeah, you don't need to rename basic.sh.