diff --git a/src/basic/string-util.c b/src/basic/string-util.c index efd690b076..26602ac771 100644 --- a/src/basic/string-util.c +++ b/src/basic/string-util.c @@ -414,8 +414,10 @@ char* ellipsize_mem(const char *s, size_t old_length, size_t new_length, unsigne bool has_ansi_seq = string_has_ansi_sequence(s, old_length); - /* If no multibyte characters or ANSI sequences, use ascii_ellipsize_mem for speed */ - if (!has_ansi_seq && ascii_is_valid_n(s, old_length)) + /* If no multibyte characters or ANSI sequences, use ascii_ellipsize_mem for speed. It equates + * bytes with character cells, which holds for ASCII except for the tab, which + * unichar_console_width() counts as 8 cells. */ + if (!has_ansi_seq && ascii_is_valid_n(s, old_length) && !memchr(s, '\t', old_length)) return ascii_ellipsize_mem(s, old_length, new_length, percent); x = (new_length - 1) * percent / 100; @@ -436,7 +438,7 @@ char* ellipsize_mem(const char *s, size_t old_length, size_t new_length, unsigne char32_t c; assert_se(utf8_encoded_to_unichar(i, &c) == r); - int w = unichar_iswide(c) ? 2 : 1; + int w = unichar_console_width(c); if (k + w > x) break; @@ -467,7 +469,7 @@ char* ellipsize_mem(const char *s, size_t old_length, size_t new_length, unsigne if (!tt) return NULL; - w = unichar_iswide(c) ? 2 : 1; + w = unichar_console_width(c); if (k + w > new_length) break; diff --git a/src/basic/strv.c b/src/basic/strv.c index 443ae9028d..faf413e8ba 100644 --- a/src/basic/strv.c +++ b/src/basic/strv.c @@ -8,6 +8,7 @@ #include "escape.h" #include "extract-word.h" #include "fileio.h" +#include "gunicode.h" #include "hashmap.h" #include "log.h" #include "memory-util.h" @@ -1281,13 +1282,14 @@ int strv_rebreak_lines(char **l, size_t width, char ***ret) { in_prefix = false; } - char32_t c; - int n = utf8_encoded_to_unichar(p, &c); - if (n < 0) - return log_debug_errno(n, "Line to break contains invalid UTF-8, refusing: %m"); - - int cw = unichar_console_width(c); - assert(cw >= 0); + /* Measure with the same helper utf8_console_width() uses. Our callers mix the + * two: sd-varlink-idl.c and verbs.c size their indent with utf8_console_width() + * and then break the body here, so if we accepted and measured what that one + * refuses we'd be computing a single width from two ideas of what valid UTF-8 + * is. */ + int cw = utf8_char_console_width(p); + if (cw < 0) + return log_debug_errno(cw, "Line to break contains invalid UTF-8, refusing: %m"); w += cw; @@ -1312,7 +1314,7 @@ int strv_rebreak_lines(char **l, size_t width, char ***ret) { continue; } - p += n; + p = utf8_next_char(p); } /* Process rest of the line, except if all that's left after the last newline we already diff --git a/src/basic/utf8.c b/src/basic/utf8.c index 0fccc4711c..c4298cfe76 100644 --- a/src/basic/utf8.c +++ b/src/basic/utf8.c @@ -181,10 +181,16 @@ int utf8_char_console_width(const char *str) { assert(str); - r = utf8_encoded_to_unichar(str, &c); + /* Validate the same way utf8_is_valid() does. utf8_encoded_to_unichar() on its own accepts + * overlong encodings, surrogates, noncharacters and the obsolete 5 and 6 byte forms, which would + * make our idea of how wide a string is disagree with our idea of whether it is valid UTF-8 in + * the first place. */ + r = utf8_encoded_valid_unichar(str, SIZE_MAX); if (r < 0) return r; + assert_se(utf8_encoded_to_unichar(str, &c) == r); + return unichar_console_width(c); } diff --git a/src/shared/format-table.c b/src/shared/format-table.c index 88d76b67f0..518ef5bd25 100644 --- a/src/shared/format-table.c +++ b/src/shared/format-table.c @@ -2079,18 +2079,23 @@ static char* align_string_mem(const char *str, const char *url, size_t new_lengt } else clickable_length = old_length; - /* Determine current width on screen */ + /* Determine current width on screen. Use the same helper the measuring pass uses, so that this + * function cannot keep a second, more permissive idea of both validity and width of its own. + * Note that our caller only gets here after utf8_console_width() succeeded on the very same + * string, so the invalid case below should not be reachable in practice. */ p = str; while (p < str + old_length) { - char32_t c; - - if (utf8_encoded_to_unichar(p, &c) < 0) { - p++, w++; /* count invalid chars as 1 */ + int n = utf8_char_console_width(p); + if (n < 0) { + /* Count an invalid sequence as one character of width one. Skip all of it, so + * that its continuation bytes are not charged a cell each of their own. */ + p = utf8_next_char(p); + w++; continue; } p = utf8_next_char(p); - w += unichar_iswide(c) ? 2 : 1; + w += n; } /* Already wider than the target, if so, don't do anything */ diff --git a/src/test/test-format-table.c b/src/test/test-format-table.c index 4c96d6caf0..0b3ec62953 100644 --- a/src/test/test-format-table.c +++ b/src/test/test-format-table.c @@ -8,9 +8,12 @@ #include "env-util.h" #include "format-table.h" #include "json-util.h" +#include "string-util.h" +#include "strv.h" #include "terminal-util.h" #include "tests.h" #include "time-util.h" +#include "utf8.h" TEST(issue_9549) { _cleanup_(table_unrefp) Table *table = NULL; @@ -35,6 +38,89 @@ TEST(issue_9549) { "foooo raw no 673.6M Wed 2018-07-11 00:10:33 J… Wed 2018-07-11 00:16:00 JST\n"); } +TEST(invalid_utf8_cell) { + /* A cell that is not valid UTF-8 must be refused the same way whatever the column width is. + * The width was previously determined with a more permissive decoder than the one ellipsize() + * uses, so a surrogate, overlong, noncharacter or obsolete five byte sequence was measured as + * if it were fine, and then failed inside ellipsize() as a bogus -ENOMEM, but only once the + * column happened to be narrow enough to require truncation. The widths below deliberately go + * past the width of the whole table, so that the last iterations are ones where no truncation + * is needed at all: those are the ones where the old code did not fail but printed the invalid + * sequence to the terminal instead. */ + + FOREACH_STRING(x, + "\xed\xa0\x80", /* UTF-16 surrogate */ + "\xc0\xaf", /* overlong '/' */ + "\xef\xb7\x90", /* noncharacter U+FDD0 */ + "\xf8\x88\x80\x80\x80") /* obsolete five byte form */ + for (size_t w = 8; w <= 64; w += 8) { + _cleanup_(table_unrefp) Table *table = NULL; + _cleanup_free_ char *formatted = NULL, *cell = NULL; + + ASSERT_NOT_NULL((table = table_new("name", "value"))); + ASSERT_NOT_NULL((cell = strjoin("aaaaaaaaaaaaaaaa", x, "aaaaaaaaaaaaaaaa"))); + ASSERT_OK(table_add_many(table, + TABLE_STRING, cell, + TABLE_STRING, "second")); + table_set_width(table, w); + + ASSERT_ERROR(table_format(table, &formatted), EINVAL); + } +} + +TEST(tab_in_cell) { + _cleanup_(table_unrefp) Table *table = NULL; + _cleanup_free_ char *formatted = NULL; + + /* A tab takes up eight character cells, so the padding align_string_mem() adds has to be + * computed with unichar_console_width() too, or the column comes out too wide. + * + * Note that unichar_console_width() charges a tab a flat eight cells, while a terminal + * advances to the next multiple of eight instead. The two only agree if the tab itself starts + * on a multiple of eight, hence the eight characters in front of it here: that way the + * expected output below is what a terminal really renders, too. */ + + ASSERT_NOT_NULL((table = table_new("name", "value"))); + ASSERT_OK(table_add_many(table, + TABLE_STRING, "aaaaaaaa\tb", + TABLE_STRING, "1", + TABLE_STRING, "wide enough to pad the tab cell", + TABLE_STRING, "2")); + + table_set_width(table, 40); + ASSERT_OK(table_format(table, &formatted)); + + printf("%s\n", formatted); + ASSERT_STREQ(formatted, + "NAME VALUE\n" + "aaaaaaaa\tb 1\n" + "wide enough to pad the tab cell 2\n"); + + /* And the case the two passes have to agree on: a cell whose tab makes it the widest one in + * its column, in a table pinned narrower than that. The cell drives the requested column width + * and is then handed to ellipsize(), which used to believe the tab was a single cell and so + * returned something that still overflowed the column it had just been asked to fit into. */ + + _cleanup_(table_unrefp) Table *narrow = NULL; + _cleanup_free_ char *narrow_formatted = NULL; + _cleanup_strv_free_ char **lines = NULL; + + ASSERT_NOT_NULL((narrow = table_new("name", "value"))); + ASSERT_OK(table_add_many(narrow, + TABLE_STRING, "aaaaaaaa\tbbbbbbbb", + TABLE_STRING, "1", + TABLE_STRING, "short", + TABLE_STRING, "2")); + + table_set_width(narrow, 20); + ASSERT_OK(table_format(narrow, &narrow_formatted)); + + printf("%s\n", narrow_formatted); + ASSERT_NOT_NULL((lines = strv_split_newlines(narrow_formatted))); + STRV_FOREACH(l, lines) + ASSERT_LE(utf8_console_width(*l), (size_t) 20); +} + TEST(multiline) { _cleanup_(table_unrefp) Table *table = NULL; _cleanup_free_ char *formatted = NULL; diff --git a/src/test/test-string-util.c b/src/test/test-string-util.c index 79b8175c34..732517541f 100644 --- a/src/test/test-string-util.c +++ b/src/test/test-string-util.c @@ -9,6 +9,7 @@ #include "string-util.h" #include "strv.h" #include "tests.h" +#include "utf8.h" #include "version.h" TEST(ellipsize_mem_ansi_short) { @@ -22,6 +23,48 @@ TEST(ellipsize_mem_ansi_short) { assert_se(c); } +TEST(ellipsize_mem_tab) { + /* unichar_console_width() counts a tab as eight character cells. Callers such as + * format-table.c measure a field with utf8_console_width() and then ask ellipsize() to fit it + * into that many columns, so ellipsize() must not count a tab as a single cell: the result + * would still overflow the column that the measurement had just asked us to fit. */ + + unsigned percent; + + FOREACH_STRING(x, + "\t", + "a\tb", + "ab\tcd", + "\t\t\t", + "a\tb\tc\td\te", + "a\t你\tb") /* tabs mixed with a wide character */ + for (size_t w = 1; w <= 24; w++) + /* Sweep the ellipsis placement too: it decides how much of the string the + * forward loop takes and how much is left to the backward one, and the two + * meet at quite different places for an eight cell tab than for a single cell + * character. */ + FOREACH_ARGUMENT(percent, 0u, 30u, 50u, 90u, 100u) { + _cleanup_free_ char *e = ASSERT_NOT_NULL(ellipsize(x, w, percent)); + + ASSERT_LE(utf8_console_width(e), w); + + /* And don't truncate what already fits. */ + if (utf8_console_width(x) <= w) + ASSERT_STREQ(e, x); + } + + /* Pin a couple of concrete results, so that a change to the assumed tab width or to where the + * ellipsis lands shows up in the diff. Note that these are in terms of the flat eight cells + * unichar_console_width() charges a tab, which is what our callers measure with; a terminal + * instead advances to the next multiple of eight, and hence needs one column less for "a\tb" + * at the start of a line. */ + _cleanup_free_ char *a = ASSERT_NOT_NULL(ellipsize("a\tb", 10, 50)); + ASSERT_STREQ(a, "a\tb"); + + _cleanup_free_ char *b = ASSERT_NOT_NULL(ellipsize("a\tb", 9, 50)); + ASSERT_STREQ(b, "a…b"); +} + TEST(xsprintf) { char buf[5]; diff --git a/src/test/test-utf8.c b/src/test/test-utf8.c index 9ab6297681..875700fa7b 100644 --- a/src/test/test-utf8.c +++ b/src/test/test-utf8.c @@ -207,13 +207,36 @@ TEST(utf8_n_codepoints) { assert_se(utf8_n_codepoints("\xF1") == SIZE_MAX); } +TEST(utf8_char_console_width) { + ASSERT_EQ(utf8_char_console_width("a"), 1); + ASSERT_EQ(utf8_char_console_width("串"), 2); + ASSERT_EQ(utf8_char_console_width("\t"), 8); + + /* How wide we think a character is must agree with whether we consider it valid UTF-8 in the + * first place. utf8_encoded_to_unichar(), which this used to be based on, accepts everything + * below except the bare continuation byte, while utf8_is_valid() rejects all of them. */ + FOREACH_STRING(x, + "\x80", /* bare continuation byte */ + "\xed\xa0\x80", /* UTF-16 surrogate */ + "\xc0\xaf", /* overlong '/' */ + "\xef\xb7\x90", /* noncharacter U+FDD0 */ + "\xef\xbf\xbe", /* noncharacter U+FFFE */ + "\xf8\x88\x80\x80\x80", /* obsolete five byte form */ + "\xfc\x84\x80\x80\x80\x80") { /* obsolete six byte form */ + ASSERT_FALSE(utf8_is_valid(x)); + ASSERT_ERROR(utf8_char_console_width(x), EINVAL); + ASSERT_EQ(utf8_console_width(x), SIZE_MAX); + } +} + TEST(utf8_console_width) { - assert_se(utf8_console_width("abc") == 3); - assert_se(utf8_console_width("zażółcić gęślą jaźń") == 19); - assert_se(utf8_console_width("串") == 2); - assert_se(utf8_console_width("") == 0); - assert_se(utf8_console_width("…👊🔪💐…") == 8); - assert_se(utf8_console_width("\xF1") == SIZE_MAX); + ASSERT_EQ(utf8_console_width("abc"), (size_t) 3); + ASSERT_EQ(utf8_console_width("zażółcić gęślą jaźń"), (size_t) 19); + ASSERT_EQ(utf8_console_width("串"), (size_t) 2); + ASSERT_EQ(utf8_console_width(""), (size_t) 0); + ASSERT_EQ(utf8_console_width("…👊🔪💐…"), (size_t) 8); + ASSERT_EQ(utf8_console_width("\xF1"), SIZE_MAX); + ASSERT_EQ(utf8_console_width("\t"), (size_t) 8); } TEST(utf8_to_utf16) {