Col refactor #1115

kerolasa · 2020-08-06T18:53:31Z

Welcome back from vacation @karelzak

While back when d8bfcb4 was in review phase I mentioned col(1) is in need for whole bunch of attention. This pull request contains the things that I thought make sense when bringing the command from past to present. The patches start with tests that work when they are added, and do not need changes at any step on the way. I hope that gives more confidence these changes are probably ok.

evverx · 2020-08-06T19:08:22Z

@kerolasa I wonder if it would be possible to run $TS_CMD_COL with ts_run? It's the only place where ASAN_OPTIONS and UBSAN_OPTIONS are set correctly: #1072.

With these tests coverage is about 89%. The ts_run is added to ensure ASAN_OPTIONS and UBSAN_OPTIONS are set correctly when the tests run. Reference: util-linux#1115 (comment) Signed-off-by: Sami Kerola <kerolasa@iki.fi>

kerolasa · 2020-08-08T11:40:47Z

@kerolasa I wonder if it would be possible to run $TS_CMD_COL with ts_run? It's the only place where ASAN_OPTIONS and UBSAN_OPTIONS are set correctly: #1072.

I can certainly try. Lets see if 2f733b4 works better.

kerolasa · 2020-08-08T15:53:17Z

I should have taken a look what failed. The cf7825e should fix the LeakSanitizer in cost of being free() before exit fix that is usually considered quite pointless activity.

evverx · 2020-08-08T16:14:15Z

I think by analogy with #1077 (comment) another option would be to pass detect_leaks=0 when the col tests are run. Though all those memory leaks are still on Coverity Scan: https://scan.coverity.com/projects/karelzak-util-linux. I'd say going through them and figuring out what is intentional isn't exactly meaningful activity either :-)

evverx · 2020-08-08T16:20:45Z

Speaking of Coverity @kerolasa I'm not sure if you have access to it. I've just invited you to the "util-linux" organization there. I hope it's all right.

evverx · 2020-08-08T16:49:23Z

FWIW to judge from https://travis-ci.org/github/karelzak/util-linux/jobs/716144038 it seems on macOS col/io is failing with

diff-{{{

--- /Users/travis/build/karelzak/util-linux/tests/expected/col/io	2020-08-08 16:10:09.000000000 +0000

+++ /Users/travis/build/karelzak/util-linux/tests/output/col/io	2020-08-08 16:27:10.000000000 +0000

@@ -37,7 +37,6 @@

 half line

 �9
1

 �9exit sane

-col: failed on line 1: Invalid or incomplete multibyte or wide character

 flushing

 1

 2

}}}-diff



 FAILED (col/io)

kerolasa · 2020-08-08T17:00:58Z

Speaking of Coverity @kerolasa I'm not sure if you have access to it. I've just invited you to the "util-linux" organization there. I hope it's all right.

Thank you. Looks like there are plenty of small fixes that could be done. Perhaps I should cease the opportunity and make this release cycle to be mostly about covery fixes.

I will try to look into latest failures. Maybe the easiest is to compile the col(1) on my laptop with leaksanitizer enabled and make the feedback loop a lot shorter.

evverx · 2020-08-08T17:13:32Z

@kerolasa apart from Coverity util-linux was integrated into OSS-Fuzz recently (I'll try to start covering more code once #1068 is merged). If you're interested in receiving OSS-Fuzz bug reports let me know so that I can add your email address to https://github.com/google/oss-fuzz/blob/master/projects/util-linux/project.yaml. I should add that non-gmail addresses can only be used to get email notifications. To log in to their bug tracker and to oss-fuzz.com gmail addresses should be used unfortunately.

kerolasa · 2020-08-08T17:21:19Z

@evverx I have no idea what to expect with OSS-Fuzz but I am sure the reports themselves will educate me how to use them. My addresses are kerolasa(at)iki.fi and kerolasa(at)gmail.com. Thank you Evgeny, really nice to see another active contributor working to get util-linux better.

With these tests coverage is about 89%. The ts_run is added to ensure ASAN_OPTIONS and UBSAN_OPTIONS are set correctly when the tests run. Reference: util-linux#1115 (comment) Signed-off-by: Sami Kerola <kerolasa@iki.fi>

evverx · 2020-08-08T19:02:50Z

I have no idea what to expect with OSS-Fuzz

OSS-Fuzz has found two issues so far: https://bugs.chromium.org/p/oss-fuzz/issues/list?q=label%3AProj-util-linux&can=1. https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=23861&q=label%3AProj-util-linux&can=1 seems to be a bug in the fuzz target though. Looks like I forgot to limit the number of bytes it's supposed to handle. I'll fix it next week.

My addresses are ...

I've just opened google/oss-fuzz#4287.

With these tests coverage is about 89%. The ts_run is added to ensure ASAN_OPTIONS and UBSAN_OPTIONS are set correctly when the tests run. Reference: util-linux#1115 (comment) Signed-off-by: Sami Kerola <kerolasa@iki.fi>

kerolasa · 2020-08-09T17:26:07Z

Hopefully last fix. The col io test "exit sane" is now discarding error message about invalid multibyte because linux and macos does not share the same error message string.

kerolasa · 2020-08-17T20:55:00Z

Really weird, why MacOS is adding newline?

https://travis-ci.org/github/karelzak/util-linux/jobs/716341634#L3804

With these tests coverage is about 89%. The ts_run is added to ensure ASAN_OPTIONS and UBSAN_OPTIONS are set correctly when the tests run. Reference: util-linux#1115 (comment) Signed-off-by: Sami Kerola <kerolasa@iki.fi>

kerolasa · 2020-08-19T19:57:24Z

The part of test that upset macos is removed.

With these tests coverage is about 89%. The ts_run is added to ensure ASAN_OPTIONS and UBSAN_OPTIONS are set correctly when the tests run. Reference: util-linux#1115 (comment) Signed-off-by: Sami Kerola <kerolasa@iki.fi>

karelzak · 2020-09-02T10:18:13Z

text-utils/col.c

 {
-	errx(EXIT_FAILURE, _("write error"));
+	if (putwchar(ch) == WEOF)
+		errx(EXIT_FAILURE, _("write error"));


Would be better to use _("write failed") as we have in other tools? Would be possible to use err() to get details about the error?

Text is fixed in 3375fd9, and I checked the err() is used not the errx().

karelzak · 2020-09-02T10:30:49Z

text-utils/col.c

 	}
-	nb /= 2;
-	for (i = nb; --i >= 0;)
+	ctl->nblank_lines /= 2;


Is it correct? It seems you modify global ctl->nblank_lines, but the original code uses a local variable and it does not refresh the original global setting -- nblank_lines is unmodified by this function in original code.

Good point. I removed the commit that had ill thought variable removal.

karelzak · 2020-09-02T10:38:57Z

text-utils/col.c

@@ -104,19 +104,20 @@ struct line_str {
 	CHAR	*l_line;		/* characters on the line */
 	LINE	*l_prev;		/* previous line */
 	LINE	*l_next;		/* next line */


It would be nice by a separate patch to remove all the typedefs and LINE and CHAR, and use struct col_line and struct col_char. The typedef makes sense for simple opaque types (like some numbers etc.), otherwise typedef is evil.

Good idea, typedefs are removed in 2446db1.

karelzak · 2020-09-02T10:41:29Z

text-utils/col.c

@@ -173,7 +176,7 @@ static void __attribute__((__noreturn__)) usage(void)
 static inline void col_putchar(wchar_t ch)
 {
 	if (putwchar(ch) == WEOF)
-		errx(EXIT_FAILURE, _("write error"));
+		err(EXIT_FAILURE, _("write error"));


Ah, yes, err() -- thanks.

karelzak · 2020-09-02T10:46:54Z

text-utils/col.c

@@ -651,5 +682,6 @@ int main(int argc, char **argv)
 		/* missing a \n on the last line? */
 		ctl.nblank_lines = 2;
 	flush_blanks(&ctl);
+	free_line_allocations(ctl.alloc_root);


Maybe we can use some #ifdef to call free_line_allocations() only to make LeakSanitizer happy :-)

Lets see if 3252914 works as expected. I added #if defined(__SANITIZE_ADDRESS__) around code that is performing these pointless at exit free's.

With these tests coverage is about 89%. The ts_run is added to ensure ASAN_OPTIONS and UBSAN_OPTIONS are set correctly when the tests run. Reference: util-linux#1115 (comment) Signed-off-by: Sami Kerola <kerolasa@iki.fi>

Signed-off-by: Sami Kerola <kerolasa@iki.fi>

Mark --tabs and --spaces mutually exclusive in same go. Signed-off-by: Sami Kerola <kerolasa@iki.fi>

Signed-off-by: Sami Kerola <kerolasa@iki.fi>

Left side is always smaller or equal to right side. This makes reading code quicker when not having to constantly swap where is the greater value. Signed-off-by: Sami Kerola <kerolasa@iki.fi>

Signed-off-by: Sami Kerola <kerolasa@iki.fi>

Karel Zak said; typedef is evil, see reference. I don't know are they evil, but it is fair comment structs without hiding what is the data type is easier and quicker understand when reading the code. Reference: util-linux#1115 (comment) Signed-off-by: Sami Kerola <kerolasa@iki.fi>

Clean up before exit to satisfy LeakSanitizer tests run by travis. Signed-off-by: Sami Kerola <kerolasa@iki.fi>

Karel Zak said; typedef is evil, see reference. I don't know are they evil, but it is fair comment structs without hiding what is the data type is easier and quicker understand when reading the code. Reference: util-linux#1115 (comment) Signed-off-by: Sami Kerola <kerolasa@iki.fi>

karelzak · 2020-09-29T12:29:23Z

Thanks! I did tiny changes to the code too.

The macro FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION does not have to enabled in all cases (e.g. default travis-ci, local tests, ...). It seems more robust also check for __SANITIZE_ADDRESS__ too. Addresses: #1115 Signed-off-by: Karel Zak <kzak@redhat.com>

kerolasa force-pushed the col-refactor branch from 68d85f3 to eddfeb0 Compare August 8, 2020 11:39

kerolasa force-pushed the col-refactor branch from cf7825e to b6fc46a Compare August 8, 2020 17:52

kerolasa force-pushed the col-refactor branch from b6fc46a to 59a321b Compare August 9, 2020 17:23

kerolasa force-pushed the col-refactor branch from 59a321b to 4ad29fe Compare August 19, 2020 18:33

kerolasa force-pushed the col-refactor branch from 4ad29fe to 2bc5a16 Compare August 28, 2020 19:54

karelzak reviewed Sep 2, 2020

View reviewed changes

kerolasa added 2 commits September 11, 2020 20:55

col: add more tests

b97981a

With these tests coverage is about 89%. The ts_run is added to ensure ASAN_OPTIONS and UBSAN_OPTIONS are set correctly when the tests run. Reference: util-linux#1115 (comment) Signed-off-by: Sami Kerola <kerolasa@iki.fi>

col: remove function prototypes

f5ab4ee

Signed-off-by: Sami Kerola <kerolasa@iki.fi>

kerolasa added 12 commits September 11, 2020 20:55

col: use typedef and enum to clarify struct

6c5a421

Signed-off-by: Sami Kerola <kerolasa@iki.fi>

col: use inline function rather than function like define

fd8270b

Signed-off-by: Sami Kerola <kerolasa@iki.fi>

col: move global variables to a control structure

31a61cb

Signed-off-by: Sami Kerola <kerolasa@iki.fi>

col: move option handling to separate function

9f60a69

Mark --tabs and --spaces mutually exclusive in same go. Signed-off-by: Sami Kerola <kerolasa@iki.fi>

col: initialize variables when they are declared

d38392a

Signed-off-by: Sami Kerola <kerolasa@iki.fi>

col: add handle_not_graphic() function

6591d3b

Signed-off-by: Sami Kerola <kerolasa@iki.fi>

col: add update_cur_line() function

812e849

Signed-off-by: Sami Kerola <kerolasa@iki.fi>

col: add structure to hold line variables

0148d75

Signed-off-by: Sami Kerola <kerolasa@iki.fi>

col: use size_t when dealing with numbers that buffer sizes

8f36d39

Signed-off-by: Sami Kerola <kerolasa@iki.fi>

col: flip all comparisions to numerical order

e15ed08

Left side is always smaller or equal to right side. This makes reading code quicker when not having to constantly swap where is the greater value. Signed-off-by: Sami Kerola <kerolasa@iki.fi>

col: add defaults to switch case clauses

12234f4

Signed-off-by: Sami Kerola <kerolasa@iki.fi>

col: tidy up sources a little bit

3375fd9

Signed-off-by: Sami Kerola <kerolasa@iki.fi>

kerolasa force-pushed the col-refactor branch from 2bc5a16 to 2446db1 Compare September 12, 2020 18:21

kerolasa added 2 commits September 12, 2020 23:19

col: free memory before exit [LeakSanitizer]

86c6d3f

Clean up before exit to satisfy LeakSanitizer tests run by travis. Signed-off-by: Sami Kerola <kerolasa@iki.fi>

kerolasa force-pushed the col-refactor branch from 2446db1 to 81c9867 Compare September 12, 2020 22:21

karelzak merged commit 18b96d7 into util-linux:master Sep 29, 2020

kerolasa deleted the col-refactor branch September 29, 2020 19:49

kerolasa mentioned this pull request Oct 18, 2020

Ul refactor #1165

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Col refactor #1115

Col refactor #1115

kerolasa commented Aug 6, 2020

evverx commented Aug 6, 2020

kerolasa commented Aug 8, 2020

kerolasa commented Aug 8, 2020

evverx commented Aug 8, 2020

evverx commented Aug 8, 2020

evverx commented Aug 8, 2020

kerolasa commented Aug 8, 2020

evverx commented Aug 8, 2020

kerolasa commented Aug 8, 2020

evverx commented Aug 8, 2020

kerolasa commented Aug 9, 2020

kerolasa commented Aug 17, 2020

kerolasa commented Aug 19, 2020

karelzak Sep 2, 2020

kerolasa Sep 12, 2020

karelzak Sep 2, 2020

kerolasa Sep 12, 2020

karelzak Sep 2, 2020

kerolasa Sep 12, 2020

karelzak Sep 2, 2020

karelzak Sep 2, 2020

kerolasa Sep 12, 2020

karelzak commented Sep 29, 2020

Col refactor #1115

Col refactor #1115

Conversation

kerolasa commented Aug 6, 2020

evverx commented Aug 6, 2020

kerolasa commented Aug 8, 2020

kerolasa commented Aug 8, 2020

evverx commented Aug 8, 2020

evverx commented Aug 8, 2020

evverx commented Aug 8, 2020

kerolasa commented Aug 8, 2020

evverx commented Aug 8, 2020

kerolasa commented Aug 8, 2020

evverx commented Aug 8, 2020

kerolasa commented Aug 9, 2020

kerolasa commented Aug 17, 2020

kerolasa commented Aug 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karelzak commented Sep 29, 2020