From 0c532fa783352422d45270df12bee49344a2dc5e Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Tue, 8 Nov 2022 01:42:27 -0500 Subject: [PATCH 01/12] scanner: only abort() if there is a bug abort() is for internal errors that indicate bugs, or for catastrophic failures such as out-of-memory. It is not for I/O errors or bad XML files. Signed-off-by: Demi Marie Obenour --- src/scanner.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/scanner.c b/src/scanner.c index 3cd05d13..5a323405 100644 --- a/src/scanner.c +++ b/src/scanner.c @@ -129,7 +129,7 @@ is_dtd_valid(FILE *input, const char *filename) doc = xmlCtxtReadFd(ctx, fd, filename, NULL, 0); if (!doc) { fprintf(stderr, "Failed to read XML\n"); - abort(); + exit(EXIT_FAILURE); } rc = xmlValidateDtd(dtdctx, doc, dtd); @@ -141,7 +141,7 @@ is_dtd_valid(FILE *input, const char *filename) if (lseek(fd, 0, SEEK_SET) != 0) { fprintf(stderr, "Failed to reset fd, output would be garbage.\n"); - abort(); + exit(EXIT_FAILURE); } #endif return rc; From cd5b4bcca773d888c3cd690fea367b748a58ab6c Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Tue, 8 Nov 2022 01:43:48 -0500 Subject: [PATCH 02/12] scanner: add missing attributes These allow better checking by the compiler. Signed-off-by: Demi Marie Obenour --- src/scanner.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/scanner.c b/src/scanner.c index 5a323405..92acab1c 100644 --- a/src/scanner.c +++ b/src/scanner.c @@ -380,7 +380,7 @@ desc_dump(char *desc, const char *fmt, ...) putchar('\n'); } -static void __attribute__ ((noreturn)) +static void __attribute__((format(printf, 2, 3), noreturn)) fail(struct location *loc, const char *msg, ...) { va_list ap; @@ -394,7 +394,7 @@ fail(struct location *loc, const char *msg, ...) exit(EXIT_FAILURE); } -static void +static void __attribute__((format(printf, 2, 3))) warn(struct location *loc, const char *msg, ...) { va_list ap; @@ -836,7 +836,7 @@ start_element(void *data, const char *element_name, const char **atts) switch (arg->type) { case NEW_ID: ctx->message->new_id_count++; - /* fallthrough */ + __attribute__((fallthrough)); case OBJECT: if (interface_name) { validate_identifier(&ctx->loc, From 5802a083b48218361c5c59da3cd43c01274b462d Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Tue, 8 Nov 2022 01:44:41 -0500 Subject: [PATCH 03/12] scanner: Fail on more invalid XML files These situations can happen if the XML is not valid according to the Wayland DTD. Signed-off-by: Demi Marie Obenour --- src/scanner.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/scanner.c b/src/scanner.c index 92acab1c..6ac67243 100644 --- a/src/scanner.c +++ b/src/scanner.c @@ -784,6 +784,9 @@ start_element(void *data, const char *element_name, const char **atts) if (version == 0) fail(&ctx->loc, "no interface version given"); + if (atts[4]) + fail(&ctx->loc, "unexpected attribute on interface"); + validate_identifier(&ctx->loc, name, STANDALONE_IDENT); interface = create_interface(ctx->loc, name, version); ctx->interface = interface; @@ -924,6 +927,9 @@ start_element(void *data, const char *element_name, const char **atts) } else if (strcmp(element_name, "description") == 0) { if (summary == NULL) fail(&ctx->loc, "description without summary"); + /* must be valid since summary attribute present */ + if (atts[2]) + fail(&ctx->loc, "description with non-summary attribute"); description = xzalloc(sizeof *description); description->summary = xstrdup(summary); @@ -939,6 +945,8 @@ start_element(void *data, const char *element_name, const char **atts) else ctx->protocol->description = description; ctx->description = description; + } else { + fail(&ctx->loc, "unknown element %s", element_name); } } From 3dc97759fd842d7d766df593c19ea3d76d98f0c0 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Tue, 8 Nov 2022 01:46:50 -0500 Subject: [PATCH 04/12] scanner: Fail on invalid attribute names These should be caught by the DTD check, but failed DTD validation is non-fatal by default. Signed-off-by: Demi Marie Obenour --- src/scanner.c | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/src/scanner.c b/src/scanner.c index 6ac67243..19f566d4 100644 --- a/src/scanner.c +++ b/src/scanner.c @@ -742,29 +742,30 @@ start_element(void *data, const char *element_name, const char **atts) for (i = 0; atts[i]; i += 2) { if (strcmp(atts[i], "name") == 0) name = atts[i + 1]; - if (strcmp(atts[i], "version") == 0) { + else if (strcmp(atts[i], "version") == 0) { version = strtouint(atts[i + 1]); if (version == -1) fail(&ctx->loc, "wrong version (%s)", atts[i + 1]); - } - if (strcmp(atts[i], "type") == 0) + } else if (strcmp(atts[i], "type") == 0) type = atts[i + 1]; - if (strcmp(atts[i], "value") == 0) + else if (strcmp(atts[i], "value") == 0) value = atts[i + 1]; - if (strcmp(atts[i], "interface") == 0) + else if (strcmp(atts[i], "interface") == 0) interface_name = atts[i + 1]; - if (strcmp(atts[i], "summary") == 0) + else if (strcmp(atts[i], "summary") == 0) summary = atts[i + 1]; - if (strcmp(atts[i], "since") == 0) + else if (strcmp(atts[i], "since") == 0) since = atts[i + 1]; - if (strcmp(atts[i], "deprecated-since") == 0) + else if (strcmp(atts[i], "deprecated-since") == 0) deprecated_since = atts[i + 1]; - if (strcmp(atts[i], "allow-null") == 0) + else if (strcmp(atts[i], "allow-null") == 0) allow_null = atts[i + 1]; - if (strcmp(atts[i], "enum") == 0) + else if (strcmp(atts[i], "enum") == 0) enumeration_name = atts[i + 1]; - if (strcmp(atts[i], "bitfield") == 0) + else if (strcmp(atts[i], "bitfield") == 0) bitfield = atts[i + 1]; + else + fail(&ctx->loc, "invalid attribute name (%s)", atts[i]); } ctx->character_data_length = 0; From 60b34a6fcbc53820d5cf340fa2c282811b306d9c Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Tue, 8 Nov 2022 01:50:01 -0500 Subject: [PATCH 05/12] scanner: Stricter strtouint This helps catch bad version numbers. Signed-off-by: Demi Marie Obenour --- src/scanner.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/src/scanner.c b/src/scanner.c index 19f566d4..3f7bc1fe 100644 --- a/src/scanner.c +++ b/src/scanner.c @@ -616,20 +616,26 @@ strtouint(const char *str) { long int ret; char *end; - int prev_errno = errno; + int prev_errno; + if (str[0] == '0') + return str[1] ? -1 : 0; + if (str[0] < '1' || str[0] > '9') + return -1; + + prev_errno = errno; errno = 0; ret = strtol(str, &end, 10); - if (errno != 0 || end == str || *end != '\0') - return -1; - - /* check range */ - if (ret < 0 || ret > INT_MAX) { + if (errno != 0 || end == str || *end != '\0') { + errno = prev_errno; return -1; } - errno = prev_errno; - return (int)ret; + + if (ret <= 0) + abort(); /* caught by syntax checks */ + + return ret > INT_MAX ? -1 : ret; } /* Check that the provided string will produce valid "C" identifiers. From 2a122485b3899e70f56aa27065caeed1ec3ec715 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Sat, 10 Aug 2024 18:56:24 -0400 Subject: [PATCH 06/12] scanner: Reject version 0 It is not valid in Wayland. Signed-off-by: Demi Marie Obenour --- src/scanner.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/scanner.c b/src/scanner.c index 3f7bc1fe..241755b6 100644 --- a/src/scanner.c +++ b/src/scanner.c @@ -752,6 +752,8 @@ start_element(void *data, const char *element_name, const char **atts) version = strtouint(atts[i + 1]); if (version == -1) fail(&ctx->loc, "wrong version (%s)", atts[i + 1]); + if (version == 0) + fail(&ctx->loc, "version 0 is invalid"); } else if (strcmp(atts[i], "type") == 0) type = atts[i + 1]; else if (strcmp(atts[i], "value") == 0) From b39e45c48d12538366cb7b27735f3fcdc34c768d Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Sat, 10 Aug 2024 18:57:25 -0400 Subject: [PATCH 07/12] scanner: Reject attributes that are not valid for an element This would be caught by the DTD but DTD validation is not fatal by default. Signed-off-by: Demi Marie Obenour --- src/scanner.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/scanner.c b/src/scanner.c index 241755b6..82c51443 100644 --- a/src/scanner.c +++ b/src/scanner.c @@ -770,22 +770,27 @@ start_element(void *data, const char *element_name, const char **atts) allow_null = atts[i + 1]; else if (strcmp(atts[i], "enum") == 0) enumeration_name = atts[i + 1]; - else if (strcmp(atts[i], "bitfield") == 0) + else if (strcmp(atts[i], "bitfield") == 0) { + if (strcmp(element_name, "enum")) + fail(&ctx->loc, "bitfield attribute only valid on enum element"); bitfield = atts[i + 1]; - else + } else { fail(&ctx->loc, "invalid attribute name (%s)", atts[i]); + } } ctx->character_data_length = 0; if (strcmp(element_name, "protocol") == 0) { if (name == NULL) fail(&ctx->loc, "no protocol name given"); - + if (atts[2]) + fail(&ctx->loc, "Unexpected attribute for protocol element"); validate_identifier(&ctx->loc, name, STANDALONE_IDENT); ctx->protocol->name = xstrdup(name); ctx->protocol->uppercase_name = uppercase_dup(name); } else if (strcmp(element_name, "copyright") == 0) { - + if (atts[0]) + fail(&ctx->loc, "copyright element takes no attributes"); } else if (strcmp(element_name, "interface") == 0) { if (name == NULL) fail(&ctx->loc, "no interface name given"); From b5d84ea6fe240ceb030fcd554c60acedc6c3cf71 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Sat, 10 Aug 2024 18:57:59 -0400 Subject: [PATCH 08/12] scanner: fail if since versions are not increasing They will result in invalid code being generated. Signed-off-by: Demi Marie Obenour --- src/scanner.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/scanner.c b/src/scanner.c index 82c51443..3271543d 100644 --- a/src/scanner.c +++ b/src/scanner.c @@ -826,7 +826,7 @@ start_element(void *data, const char *element_name, const char **atts) version = version_from_since(ctx, since); if (version < ctx->interface->since) - warn(&ctx->loc, "since version not increasing\n"); + fail(&ctx->loc, "since version not increasing\n"); ctx->interface->since = version; message->since = version; From 81bbde007e6f07e0c962f5ae785aef2650336f69 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Sun, 7 Jan 2024 13:23:06 -0500 Subject: [PATCH 09/12] scanner: Refuse types other than "destructor" Previously they were ignored. Signed-off-by: Demi Marie Obenour --- src/scanner.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/scanner.c b/src/scanner.c index 3271543d..f13ef6a5 100644 --- a/src/scanner.c +++ b/src/scanner.c @@ -821,8 +821,11 @@ start_element(void *data, const char *element_name, const char **atts) wl_list_insert(ctx->interface->event_list.prev, &message->link); - if (type != NULL && strcmp(type, "destructor") == 0) + if (type != NULL) { + if (strcmp(type, "destructor") != 0) + fail(&ctx->loc, "type of request or event must be \"destructor\" if specified"); message->destructor = 1; + } version = version_from_since(ctx, since); if (version < ctx->interface->since) From 8e83f0f531300d498090bfc4658b1b4767cdc045 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Mon, 12 Aug 2024 00:03:24 -0400 Subject: [PATCH 10/12] scanner: Remove unreachable call to exit() fail() already does not return. Signed-off-by: Demi Marie Obenour --- src/scanner.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/scanner.c b/src/scanner.c index f13ef6a5..99482a5f 100644 --- a/src/scanner.c +++ b/src/scanner.c @@ -1232,7 +1232,6 @@ emit_stubs(struct wl_list *message_list, struct interface *interface) "interface '%s' has method named destroy " "but no destructor", interface->name); - exit(EXIT_FAILURE); } if (!has_destroy && strcmp(interface->name, "wl_display") != 0) { From 60922eb8bd2226ae865eb1d9804df3a4950c8a5b Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Sat, 27 Jul 2024 01:11:08 -0400 Subject: [PATCH 11/12] scanner: Validate element nesting This validates that each element is nested inside the correct parent element. The DTD already checks for this, but DTD checking is not fatal by default and is only possible if libwayland is built with libxml2 support. There are a few bugs, fixed in the next commit. Signed-off-by: Demi Marie Obenour --- src/scanner.c | 186 +++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 145 insertions(+), 41 deletions(-) diff --git a/src/scanner.c b/src/scanner.c index 99482a5f..bea765e7 100644 --- a/src/scanner.c +++ b/src/scanner.c @@ -154,9 +154,23 @@ struct location { int line_number; }; +enum element { + INVALID, + PROTOCOL, + COPYRIGHT, + INTERFACE, + REQUEST, + EVENT, + ENUM, + ENTRY, + ARG, + DESCRIPTION, +}; + struct description { char *summary; char *text; + enum element parent; }; struct protocol { @@ -196,6 +210,7 @@ struct message { int destructor; int since, deprecated_since; struct description *description; + enum element direction; }; enum arg_type { @@ -217,6 +232,7 @@ struct arg { struct wl_list link; char *summary; char *enumeration_name; + enum element parent; }; struct enumeration { @@ -250,8 +266,67 @@ struct parse_context { struct description *description; char character_data[8192]; unsigned int character_data_length; + enum element parent; + bool copyright_forbidden; }; +static void __attribute__((format(printf, 2, 3), noreturn)) +fail(struct location *loc, const char *msg, ...) +{ + va_list ap; + + va_start(ap, msg); + fprintf(stderr, "%s:%d: error: ", + loc->filename, loc->line_number); + vfprintf(stderr, msg, ap); + fprintf(stderr, "\n"); + va_end(ap); + exit(EXIT_FAILURE); +} + +static enum element +parse_element_name(struct parse_context *ctx, + const char *element_name) +{ + switch (*element_name) { + case 'p': + if (strcmp(element_name + 1, "rotocol") == 0) + return PROTOCOL; + break; + case 'c': + if (strcmp(element_name + 1, "opyright") == 0) + return COPYRIGHT; + break; + case 'i': + if (strcmp(element_name + 1, "nterface") == 0) + return INTERFACE; + break; + case 'd': + if (strcmp(element_name + 1, "escription") == 0) + return DESCRIPTION; + break; + case 'r': + if (strcmp(element_name + 1, "equest") == 0) + return REQUEST; + break; + case 'e': + if (strcmp(element_name + 1, "vent") == 0) + return EVENT; + if (strcmp(element_name + 1, "num") == 0) + return ENUM; + if (strcmp(element_name + 1, "ntry") == 0) + return ENTRY; + break; + case 'a': + if (strcmp(element_name + 1, "rg") == 0) + return ARG; + break; + default: + break; + } + fail(&ctx->loc, "unknown element %s", element_name); +} + enum identifier_role { STANDALONE_IDENT, TRAILING_IDENT @@ -380,20 +455,6 @@ desc_dump(char *desc, const char *fmt, ...) putchar('\n'); } -static void __attribute__((format(printf, 2, 3), noreturn)) -fail(struct location *loc, const char *msg, ...) -{ - va_list ap; - - va_start(ap, msg); - fprintf(stderr, "%s:%d: error: ", - loc->filename, loc->line_number); - vfprintf(stderr, msg, ap); - fprintf(stderr, "\n"); - va_end(ap); - exit(EXIT_FAILURE); -} - static void __attribute__((format(printf, 2, 3))) warn(struct location *loc, const char *msg, ...) { @@ -421,7 +482,7 @@ is_nullable_type(struct arg *arg) } static struct message * -create_message(struct location loc, const char *name) +create_message(struct location loc, const char *name, enum element direction) { struct message *message; @@ -429,6 +490,7 @@ create_message(struct location loc, const char *name) message->loc = loc; message->name = xstrdup(name); message->uppercase_name = uppercase_dup(name); + message->direction = direction; wl_list_init(&message->arg_list); return message; @@ -743,8 +805,13 @@ start_element(void *data, const char *element_name, const char **atts) const char *enumeration_name = NULL; const char *bitfield = NULL; int i, version = 0; + enum element element = parse_element_name(ctx, element_name); ctx->loc.line_number = XML_GetCurrentLineNumber(ctx->parser); + if (ctx->description) + fail(&ctx->loc, "element not allowed in "); + if (ctx->parent == COPYRIGHT) + fail(&ctx->loc, "element not allowed in "); for (i = 0; atts[i]; i += 2) { if (strcmp(atts[i], "name") == 0) name = atts[i + 1]; @@ -780,7 +847,9 @@ start_element(void *data, const char *element_name, const char **atts) } ctx->character_data_length = 0; - if (strcmp(element_name, "protocol") == 0) { + if (element == PROTOCOL) { + if (ctx->parent != INVALID) + fail(&ctx->loc, "Protocol element not root element"); if (name == NULL) fail(&ctx->loc, "no protocol name given"); if (atts[2]) @@ -788,12 +857,16 @@ start_element(void *data, const char *element_name, const char **atts) validate_identifier(&ctx->loc, name, STANDALONE_IDENT); ctx->protocol->name = xstrdup(name); ctx->protocol->uppercase_name = uppercase_dup(name); - } else if (strcmp(element_name, "copyright") == 0) { + } else if (element == COPYRIGHT) { if (atts[0]) - fail(&ctx->loc, "copyright element takes no attributes"); - } else if (strcmp(element_name, "interface") == 0) { + fail(&ctx->loc, " takes no attributes"); + if (ctx->parent != PROTOCOL) + fail(&ctx->loc, " must be under "); + } else if (element == INTERFACE) { if (name == NULL) fail(&ctx->loc, "no interface name given"); + if (ctx->parent != PROTOCOL) + fail(&ctx->loc, " must be under "); if (version == 0) fail(&ctx->loc, "no interface version given"); @@ -806,15 +879,17 @@ start_element(void *data, const char *element_name, const char **atts) ctx->interface = interface; wl_list_insert(ctx->protocol->interface_list.prev, &interface->link); - } else if (strcmp(element_name, "request") == 0 || - strcmp(element_name, "event") == 0) { + ctx->copyright_forbidden = true; + } else if (element == REQUEST || element == EVENT) { if (name == NULL) - fail(&ctx->loc, "no request name given"); + fail(&ctx->loc, "no %s name given", element_name); + if (ctx->parent != INTERFACE) + fail(&ctx->loc, "<%s> not child of ", element_name); validate_identifier(&ctx->loc, name, STANDALONE_IDENT); - message = create_message(ctx->loc, name); + message = create_message(ctx->loc, name, element); - if (strcmp(element_name, "request") == 0) + if (element == REQUEST) wl_list_insert(ctx->interface->request_list.prev, &message->link); else @@ -844,9 +919,11 @@ start_element(void *data, const char *element_name, const char **atts) fail(&ctx->loc, "destroy request should be destructor type"); ctx->message = message; - } else if (strcmp(element_name, "arg") == 0) { + } else if (element == ARG) { if (name == NULL) fail(&ctx->loc, "no argument name given"); + if (ctx->parent != REQUEST && ctx->parent != EVENT) + fail(&ctx->loc, " must be child of or "); validate_identifier(&ctx->loc, name, STANDALONE_IDENT); arg = create_arg(name); @@ -894,9 +971,11 @@ start_element(void *data, const char *element_name, const char **atts) wl_list_insert(ctx->message->arg_list.prev, &arg->link); ctx->message->arg_count++; - } else if (strcmp(element_name, "enum") == 0) { + } else if (element == ENUM) { if (name == NULL) fail(&ctx->loc, "no enum name given"); + if (ctx->parent != INTERFACE) + fail(&ctx->loc, " not child of "); validate_identifier(&ctx->loc, name, TRAILING_IDENT); enumeration = create_enumeration(name); @@ -914,9 +993,11 @@ start_element(void *data, const char *element_name, const char **atts) &enumeration->link); ctx->enumeration = enumeration; - } else if (strcmp(element_name, "entry") == 0) { + } else if (element == ENTRY) { if (name == NULL) fail(&ctx->loc, "no entry name given"); + if (ctx->parent != ENUM) + fail(&ctx->loc, "<%s> not child of ", element_name); validate_identifier(&ctx->loc, name, TRAILING_IDENT); entry = create_entry(name, value); @@ -941,15 +1022,16 @@ start_element(void *data, const char *element_name, const char **atts) wl_list_insert(ctx->enumeration->entry_list.prev, &entry->link); ctx->entry = entry; - } else if (strcmp(element_name, "description") == 0) { + } else if (element == DESCRIPTION) { if (summary == NULL) fail(&ctx->loc, "description without summary"); /* must be valid since summary attribute present */ if (atts[2]) - fail(&ctx->loc, "description with non-summary attribute"); + fail(&ctx->loc, "too many attributes for "); description = xzalloc(sizeof *description); description->summary = xstrdup(summary); + description->parent = ctx->parent; if (ctx->message) ctx->message->description = description; @@ -963,8 +1045,9 @@ start_element(void *data, const char *element_name, const char **atts) ctx->protocol->description = description; ctx->description = description; } else { - fail(&ctx->loc, "unknown element %s", element_name); + abort(); /* not reached */ } + ctx->parent = element; } static struct enumeration * @@ -1054,33 +1137,54 @@ end_element(void *data, const XML_Char *name) { struct parse_context *ctx = data; - if (strcmp(name, "copyright") == 0) { + switch (parse_element_name(ctx, name)) { + case COPYRIGHT: ctx->protocol->copyright = - strndup(ctx->character_data, - ctx->character_data_length); - } else if (strcmp(name, "description") == 0) { + fail_on_null(strndup(ctx->character_data, + ctx->character_data_length)); + ctx->parent = PROTOCOL; + break; + case DESCRIPTION: ctx->description->text = - strndup(ctx->character_data, - ctx->character_data_length); + fail_on_null(strndup(ctx->character_data, + ctx->character_data_length)); + ctx->parent = ctx->description->parent; ctx->description = NULL; - } else if (strcmp(name, "request") == 0 || - strcmp(name, "event") == 0) { + break; + case REQUEST: + case EVENT: ctx->message = NULL; - } else if (strcmp(name, "enum") == 0) { + ctx->parent = INTERFACE; + break; + case ENUM: if (wl_list_empty(&ctx->enumeration->entry_list)) { fail(&ctx->loc, "enumeration %s was empty", ctx->enumeration->name); } + ctx->parent = INTERFACE; ctx->enumeration = NULL; - } else if (strcmp(name, "entry") == 0) { + break; + case ENTRY: ctx->entry = NULL; - } else if (strcmp(name, "protocol") == 0) { + ctx->parent = ENUM; + break; + case PROTOCOL: { struct interface *i; wl_list_for_each(i, &ctx->protocol->interface_list, link) { verify_arguments(ctx, i, &i->request_list, &i->enumeration_list); verify_arguments(ctx, i, &i->event_list, &i->enumeration_list); } + break; + } + case ARG: + ctx->parent = ctx->message->direction; + break; + case INTERFACE: + ctx->parent = PROTOCOL; + break; + default: + abort(); } } From 1f1fb6707d6d94112db9ee5507b2f6fc0ad6499e Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Sun, 4 Aug 2024 17:58:54 -0400 Subject: [PATCH 12/12] scanner: Finish validating XML This should reject all invalid XML files instead of e.g. segfaulting. Signed-off-by: Demi Marie Obenour --- src/scanner.c | 51 +++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 43 insertions(+), 8 deletions(-) diff --git a/src/scanner.c b/src/scanner.c index bea765e7..ce824a53 100644 --- a/src/scanner.c +++ b/src/scanner.c @@ -267,7 +267,7 @@ struct parse_context { char character_data[8192]; unsigned int character_data_length; enum element parent; - bool copyright_forbidden; + bool copyright_forbidden, description_forbidden; }; static void __attribute__((format(printf, 2, 3), noreturn)) @@ -813,9 +813,11 @@ start_element(void *data, const char *element_name, const char **atts) if (ctx->parent == COPYRIGHT) fail(&ctx->loc, "element not allowed in "); for (i = 0; atts[i]; i += 2) { - if (strcmp(atts[i], "name") == 0) + if (strcmp(atts[i], "name") == 0) { name = atts[i + 1]; - else if (strcmp(atts[i], "version") == 0) { + /* Anything with a name can have a description */ + ctx->description_forbidden = false; + } else if (strcmp(atts[i], "version") == 0) { version = strtouint(atts[i + 1]); if (version == -1) fail(&ctx->loc, "wrong version (%s)", atts[i + 1]); @@ -838,7 +840,7 @@ start_element(void *data, const char *element_name, const char **atts) else if (strcmp(atts[i], "enum") == 0) enumeration_name = atts[i + 1]; else if (strcmp(atts[i], "bitfield") == 0) { - if (strcmp(element_name, "enum")) + if (element != ENUM) fail(&ctx->loc, "bitfield attribute only valid on enum element"); bitfield = atts[i + 1]; } else { @@ -852,16 +854,21 @@ start_element(void *data, const char *element_name, const char **atts) fail(&ctx->loc, "Protocol element not root element"); if (name == NULL) fail(&ctx->loc, "no protocol name given"); + /* must be valid since name given */ if (atts[2]) fail(&ctx->loc, "Unexpected attribute for protocol element"); validate_identifier(&ctx->loc, name, STANDALONE_IDENT); ctx->protocol->name = xstrdup(name); ctx->protocol->uppercase_name = uppercase_dup(name); + } else if (ctx->parent == INVALID) { + fail(&ctx->loc, "Root element not "); } else if (element == COPYRIGHT) { if (atts[0]) fail(&ctx->loc, " takes no attributes"); if (ctx->parent != PROTOCOL) fail(&ctx->loc, " must be under "); + if (ctx->copyright_forbidden) + fail(&ctx->loc, " element not allowed here"); } else if (element == INTERFACE) { if (name == NULL) fail(&ctx->loc, "no interface name given"); @@ -871,6 +878,7 @@ start_element(void *data, const char *element_name, const char **atts) if (version == 0) fail(&ctx->loc, "no interface version given"); + /* must be valid since version and name given */ if (atts[4]) fail(&ctx->loc, "unexpected attribute on interface"); @@ -879,7 +887,6 @@ start_element(void *data, const char *element_name, const char **atts) ctx->interface = interface; wl_list_insert(ctx->protocol->interface_list.prev, &interface->link); - ctx->copyright_forbidden = true; } else if (element == REQUEST || element == EVENT) { if (name == NULL) fail(&ctx->loc, "no %s name given", element_name); @@ -902,6 +909,11 @@ start_element(void *data, const char *element_name, const char **atts) message->destructor = 1; } + /* Check for unexpected attributes */ + if (atts[(1 + message->destructor + + (since != NULL) + (deprecated_since != NULL)) * 2] != NULL) + fail(&ctx->loc, "<%s> has unexpected attributes", element_name); + version = version_from_since(ctx, since); if (version < ctx->interface->since) fail(&ctx->loc, "since version not increasing\n"); @@ -922,6 +934,8 @@ start_element(void *data, const char *element_name, const char **atts) } else if (element == ARG) { if (name == NULL) fail(&ctx->loc, "no argument name given"); + if (type == NULL) + fail(&ctx->loc, "no argument type given"); if (ctx->parent != REQUEST && ctx->parent != EVENT) fail(&ctx->loc, " must be child of or "); @@ -929,6 +943,8 @@ start_element(void *data, const char *element_name, const char **atts) arg = create_arg(name); if (!set_arg_type(arg, type)) fail(&ctx->loc, "unknown type (%s)", type); + if (atts[(2 + !!summary + !!interface_name + !!allow_null + !!enumeration_name) * 2] != NULL) + fail(&ctx->loc, "<%s> has unexpected attributes", element_name); switch (arg->type) { case NEW_ID: @@ -976,6 +992,8 @@ start_element(void *data, const char *element_name, const char **atts) fail(&ctx->loc, "no enum name given"); if (ctx->parent != INTERFACE) fail(&ctx->loc, " not child of "); + if (atts[(1 + !!since + !!bitfield) * 2] != NULL) + fail(&ctx->loc, "<%s> has unexpected attributes", element_name); validate_identifier(&ctx->loc, name, TRAILING_IDENT); enumeration = create_enumeration(name); @@ -996,8 +1014,12 @@ start_element(void *data, const char *element_name, const char **atts) } else if (element == ENTRY) { if (name == NULL) fail(&ctx->loc, "no entry name given"); + if (value == NULL) + fail(&ctx->loc, "no entry value given"); if (ctx->parent != ENUM) fail(&ctx->loc, "<%s> not child of ", element_name); + if (atts[(2 + !!since + !!summary + !!deprecated_since) * 2] != NULL) + fail(&ctx->loc, "<%s> has unexpected attributes", element_name); validate_identifier(&ctx->loc, name, TRAILING_IDENT); entry = create_entry(name, value); @@ -1028,6 +1050,8 @@ start_element(void *data, const char *element_name, const char **atts) /* must be valid since summary attribute present */ if (atts[2]) fail(&ctx->loc, "too many attributes for "); + if (ctx->description_forbidden) + fail(&ctx->loc, "description not allowed here"); description = xzalloc(sizeof *description); description->summary = xstrdup(summary); @@ -1136,6 +1160,8 @@ static void end_element(void *data, const XML_Char *name) { struct parse_context *ctx = data; + ctx->copyright_forbidden = true; + ctx->description_forbidden = true; switch (parse_element_name(ctx, name)) { case COPYRIGHT: @@ -1143,6 +1169,7 @@ end_element(void *data, const XML_Char *name) fail_on_null(strndup(ctx->character_data, ctx->character_data_length)); ctx->parent = PROTOCOL; + ctx->description_forbidden = false; break; case DESCRIPTION: ctx->description->text = @@ -1181,6 +1208,10 @@ end_element(void *data, const XML_Char *name) ctx->parent = ctx->message->direction; break; case INTERFACE: + if (wl_list_empty(&ctx->interface->request_list) && + wl_list_empty(&ctx->interface->event_list)) { + fail(&ctx->loc, "Interface defines no requests and no events"); + } ctx->parent = PROTOCOL; break; default: @@ -2163,7 +2194,7 @@ int main(int argc, char *argv[]) bool core_headers = false; bool version = false; bool strict = false; - bool fail = false; + bool bad_opt = false; int opt; enum { CLIENT_HEADER, @@ -2202,7 +2233,7 @@ int main(int argc, char *argv[]) strict = true; break; default: - fail = true; + bad_opt = true; break; } } @@ -2214,7 +2245,7 @@ int main(int argc, char *argv[]) usage(EXIT_SUCCESS); else if (version) scanner_version(EXIT_SUCCESS); - else if ((argc != 1 && argc != 3) || fail) + else if ((argc != 1 && argc != 3) || bad_opt) usage(EXIT_FAILURE); else if (strcmp(argv[0], "help") == 0) usage(EXIT_SUCCESS); @@ -2307,6 +2338,10 @@ int main(int argc, char *argv[]) } while (len > 0); XML_ParserFree(ctx.parser); + ctx.parser = NULL; + + if (wl_list_empty(&ctx.protocol->interface_list)) + fail(&ctx.loc, "Protocol defines no interfaces"); switch (mode) { case CLIENT_HEADER: