Page MenuHomeFreeBSD

col(1): fix incorrect output and segfault

Authored by on Wed, Sep 23, 7:00 PM.



If col(1) is called with -f in a simple test case, the last line of the input gets cut off:

$ printf 'hello\nworld' | col -f 

as opposed to:

$ printf 'hello\nworld' | col 

This is caused by the code not calculating the correct number of half-line feeds to add to the end of the file. This patch fixes this issue, and also changes an incorrect comment regarding the calculations.

Additionally fixes

col(1) segfaults with this simple test case:

$ printf 'hello\vworld\n' | col
Segmentation fault

This issue is caused near the end of main() (line 344), when flush_lines() is called without setting the local variable pointing at the allocated lines to a valid value afterwards. The fix is quite simple, and its reasoning is described in the comment.

I understand that revisions are intended to contain only one change, but I thought that these two issues are small enough, and close enough together, that it would be more reasonable to put them into one revision.

Test Plan

Run the new tests in the tests directory.

Diff Detail

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

Event Timeline

Would you be willing to add some simple regression tests for these fixes based on the examples in the code review description? There are some tests already in usr.bin/col/tests/ The atf-sh-api man page has info on the shell functions you can use for writing tests, and there are lots of examples in the tree.

371 ↗(On Diff #77444)

There should be a blank line here.

390 ↗(On Diff #77444)

You can drop the cast of free()'s return value - it has none. edited the test plan for this revision. (Show Details)

Added tests for the issues fixed directly by this patch.
Also added some tests for half line feeds, as I found that it was very easy to affect how they behave when changing the whitespace calculation code.

Thanks for writing tests.

I think this is ok. It took me some time to wrap my head around this code.

112 ↗(On Diff #77444)

Please fix the whitespace in the alloc_line() declaration as well.

This revision is now accepted and ready to land.Fri, Sep 25, 5:32 PM

Fixed the whitespace problem. Can't believe I didn't see it, considering I saw the other one

This revision now requires review to proceed.Fri, Sep 25, 6:02 PM
This revision was not accepted when it landed; it landed in state Needs Review.Fri, Oct 9, 3:27 PM
Closed by commit rS366577: col(1): Fix a couple of bugs (authored by markj). · Explain Why
This revision was automatically updated to reflect the committed changes.