From c25bee0ffca38a70d032712b973bd863de9157b2 Mon Sep 17 00:00:00 2001 From: Michael Stapelberg Date: Sun, 20 Aug 2017 12:54:49 +0200 Subject: [PATCH 1/6] Bugfix: check bounds before accessing memory This fixes the following issue when having an error early in the config file: ==1562==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6220000180ff at pc 0x55c837edb1d3 bp 0x7ffee7534650 sp 0x7ffee7534648 READ of size 1 at 0x6220000180ff thread T0 #0 0x55c837edb1d2 in start_of_line ../../i3/src/config_parser.c:238 #1 0x55c837edc96f in parse_config ../../i3/src/config_parser.c:493 #2 0x55c837edf527 in parse_file ../../i3/src/config_parser.c:1091 #3 0x55c837ecf14b in parse_configuration ../../i3/src/config.c:65 #4 0x55c837ed1ef4 in load_configuration ../../i3/src/config.c:230 #5 0x55c837f0a8d0 in main ../../i3/src/main.c:539 #6 0x7fb63ae042b0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x202b0) #7 0x55c837e95eb9 in _start (/home/michael/i3/build/i3+0x4beb9) 0x6220000180ff is located 1 bytes to the left of 5165-byte region [0x622000018100,0x62200001952d) allocated by thread T0 here: #0 0x7fb63e590cf8 in malloc (/usr/lib/x86_64-linux-gnu/libasan.so.3+0xc1cf8) #1 0x55c837f59aa6 in smalloc ../../i3/libi3/safewrappers.c:24 #2 0x55c837edef45 in parse_file ../../i3/src/config_parser.c:1029 #3 0x55c837ecf14b in parse_configuration ../../i3/src/config.c:65 #4 0x55c837ed1ef4 in load_configuration ../../i3/src/config.c:230 #5 0x55c837f0a8d0 in main ../../i3/src/main.c:539 #6 0x7fb63ae042b0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x202b0) --- src/config_parser.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/config_parser.c b/src/config_parser.c index e72923d6..c3684737 100644 --- a/src/config_parser.c +++ b/src/config_parser.c @@ -235,7 +235,7 @@ static void next_state(const cmdp_token *token) { * */ static const char *start_of_line(const char *walk, const char *beginning) { - while (*walk != '\n' && *walk != '\r' && walk >= beginning) { + while (walk >= beginning && *walk != '\n' && *walk != '\r') { walk--; } From dd019f59fa3e2935a29d7ad4c24020eb589e12c4 Mon Sep 17 00:00:00 2001 From: Michael Stapelberg Date: Sun, 20 Aug 2017 12:55:55 +0200 Subject: [PATCH 2/6] (Re-)initialize optional fields to empty strings --- src/config_parser.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/config_parser.c b/src/config_parser.c index c3684737..0fcdeb6e 100644 --- a/src/config_parser.c +++ b/src/config_parser.c @@ -916,6 +916,7 @@ bool parse_file(const char *f, bool use_nagbar) { } /* sscanf implicitly strips whitespace. */ + value[0] = '\0'; const bool skip_line = (sscanf(buffer, "%511s %4095[^\n]", key, value) < 1 || strlen(key) < 3); const bool comment = (key[0] == '#'); value[4095] = '\n'; @@ -938,7 +939,7 @@ bool parse_file(const char *f, bool use_nagbar) { if (strcasecmp(key, "set") == 0) { char v_key[512]; - char v_value[4096]; + char v_value[4096] = {'\0'}; if (sscanf(value, "%511s %4095[^\n]", v_key, v_value) < 1) { ELOG("Failed to parse variable specification '%s', skipping it.\n", value); @@ -953,9 +954,9 @@ bool parse_file(const char *f, bool use_nagbar) { upsert_variable(&variables, v_key, v_value); continue; } else if (strcasecmp(key, "set_from_resource") == 0) { - char res_name[512]; + char res_name[512] = {'\0'}; char v_key[512]; - char fallback[4096]; + char fallback[4096] = {'\0'}; /* Ensure that this string is terminated. For example, a user might * want a variable to be empty if the resource can't be found and From 8cc11dcb08985b980d5be41d4da5c31c652b6737 Mon Sep 17 00:00:00 2001 From: Michael Stapelberg Date: Sun, 20 Aug 2017 12:57:04 +0200 Subject: [PATCH 3/6] =?UTF-8?q?Skip=20lines=20consisting=20only=20of=20?= =?UTF-8?q?=E2=80=9Cset=E2=80=9D?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/config_parser.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/config_parser.c b/src/config_parser.c index 0fcdeb6e..24a9bbfb 100644 --- a/src/config_parser.c +++ b/src/config_parser.c @@ -937,7 +937,7 @@ bool parse_file(const char *f, bool use_nagbar) { continue; } - if (strcasecmp(key, "set") == 0) { + if (strcasecmp(key, "set") == 0 && *value != '\0') { char v_key[512]; char v_value[4096] = {'\0'}; From bb0aac6e3916061c2325f31d57f7b83dc27eb285 Mon Sep 17 00:00:00 2001 From: Michael Stapelberg Date: Sun, 20 Aug 2017 12:57:26 +0200 Subject: [PATCH 4/6] start nagbar when encountering invalid set statements related to #2564 --- src/config_parser.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/config_parser.c b/src/config_parser.c index 24a9bbfb..c88e9d1e 100644 --- a/src/config_parser.c +++ b/src/config_parser.c @@ -903,6 +903,8 @@ bool parse_file(const char *f, bool use_nagbar) { fread(current_config, 1, stbuf.st_size, fstr); rewind(fstr); + bool invalid_sets = false; + while (!feof(fstr)) { if (!continuation) continuation = buffer; @@ -943,11 +945,13 @@ bool parse_file(const char *f, bool use_nagbar) { if (sscanf(value, "%511s %4095[^\n]", v_key, v_value) < 1) { ELOG("Failed to parse variable specification '%s', skipping it.\n", value); + invalid_sets = true; continue; } if (v_key[0] != '$') { ELOG("Malformed variable assignment, name has to start with $\n"); + invalid_sets = true; continue; } @@ -968,11 +972,13 @@ bool parse_file(const char *f, bool use_nagbar) { if (sscanf(value, "%511s %511s %4095[^\n]", v_key, res_name, fallback) < 1) { ELOG("Failed to parse resource specification '%s', skipping it.\n", value); + invalid_sets = true; continue; } if (v_key[0] != '$') { ELOG("Malformed variable assignment, name has to start with $\n"); + invalid_sets = true; continue; } @@ -1088,12 +1094,12 @@ bool parse_file(const char *f, bool use_nagbar) { check_for_duplicate_bindings(context); reorder_bindings(); - if (use_nagbar && (context->has_errors || context->has_warnings)) { + if (use_nagbar && (context->has_errors || context->has_warnings || invalid_sets)) { ELOG("FYI: You are using i3 version %s\n", i3_version); if (version == 3) ELOG("Please convert your configfile first, then fix any remaining errors (see above).\n"); - start_config_error_nagbar(f, context->has_errors); + start_config_error_nagbar(f, context->has_errors || invalid_sets); } bool has_errors = context->has_errors; From c04b8592fd40461b869fa1e9306bde9ade5d95c0 Mon Sep 17 00:00:00 2001 From: Michael Stapelberg Date: Sun, 20 Aug 2017 12:58:13 +0200 Subject: [PATCH 5/6] parser: only skip set[\s], not set.* fixes #2564 --- parser-specs/config.spec | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/parser-specs/config.spec b/parser-specs/config.spec index 53828221..4aa320bf 100644 --- a/parser-specs/config.spec +++ b/parser-specs/config.spec @@ -17,7 +17,8 @@ state INITIAL: end -> error -> '#' -> IGNORE_LINE - 'set' -> IGNORE_LINE + 'set ' -> IGNORE_LINE + 'set ' -> IGNORE_LINE 'set_from_resource' -> IGNORE_LINE bindtype = 'bindsym', 'bindcode', 'bind' -> BINDING 'bar' -> BARBRACE From 1e349ae3e1656de3357628de09517f18a453a6e3 Mon Sep 17 00:00:00 2001 From: Michael Stapelberg Date: Sun, 20 Aug 2017 13:12:06 +0200 Subject: [PATCH 6/6] t/201-config-parser: update expected token list --- testcases/t/201-config-parser.t | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/testcases/t/201-config-parser.t b/testcases/t/201-config-parser.t index 159de046..fb3130d5 100644 --- a/testcases/t/201-config-parser.t +++ b/testcases/t/201-config-parser.t @@ -446,8 +446,7 @@ hide_edge_border both client.focused #4c7899 #285577 #ffffff #2e9ef4 EOT -my $expected_all_tokens = "ERROR: CONFIG: Expected one of these tokens: , '#', '" . join("', '", qw( - set +my $expected_all_tokens = "ERROR: CONFIG: Expected one of these tokens: , '#', '" . join("', '", 'set ', 'set ', qw( set_from_resource bindsym bindcode