diff mbox series

[2/2] check: Add LTP-005 null termination check on test.tags

Message ID 20211201162401.12725-2-rpalethorpe@suse.com
State Superseded
Headers show
Series [1/2] doc: Add LTP-005: Array must be sentinel terminated | expand

Commit Message

Richard Palethorpe Dec. 1, 2021, 4:24 p.m. UTC
This only adds a check for test.tags. There are other null terminated
arrays which would benefit from this check. However these appear to be
accessed on every invocation of a test. So missing the null sentinel
value will reliably result in a segfault.

This check should be relatively easy to generalise. However this would
still complicate this initial commit. Other types and variables
produce different ASTs even if the concrete syntax looks
similar.

This does not explicitly check for zero at the end of an array. For
one thing the zero is implicit when one writes '{}'. We just get an
empty expression list. When one writes '{NULL, 0}', we get an implicit
cast to 'char *'. We assume an implicit cast must be zero as the
compiler should not allow some random value to be cast to a pointer
implicitly.

Perhaps surprisingly the following will be checked for null
termination correctly:

static struct tst_tag tags[] = { ..., {} };

static struct tst_test test = {
       .tags = tags,
       ...
}

There is some logic in the code which is particularly baffling

	if (item_init->type == EXPR_POS)
		item_init = item_init->init_expr;

This helps with the above case as for some reason we only see EXPR_POS
when test.tags are initialized inline.

		if (entry->init_expr->type != EXPR_SYMBOL)
			continue;

This prevents a segfault when we encounter test.timeout which is
represented with an immediate value (EXPR_VALUE). The more corner
cases we need to handle the more lines like this appear.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---
 tools/sparse/sparse-ltp.c | 92 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 92 insertions(+)

Comments

Petr Vorel Dec. 1, 2021, 7:10 p.m. UTC | #1
Hi Richie,

Agree to start with tst_test check, we can generalize later.
LGTM.

Reviewed-by: Petr Vorel <pvorel@suse.cz>

Kind regards,
Petr
Cyril Hrubis Dec. 6, 2021, 3:49 p.m. UTC | #2
Hi!
> This does not explicitly check for zero at the end of an array. For
> one thing the zero is implicit when one writes '{}'. We just get an
> empty expression list. When one writes '{NULL, 0}', we get an implicit

I would vote for dropping the part that checks for implicit cats so that
we unify all code on {} for sentinels. I guess that all we would have to
do is to drop the loop and only check if the list is empty.

Other than that the code looks good to me:

Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
Richard Palethorpe Dec. 7, 2021, 8:22 a.m. UTC | #3
Hello,

Cyril Hrubis <chrubis@suse.cz> writes:

> Hi!
>> This does not explicitly check for zero at the end of an array. For
>> one thing the zero is implicit when one writes '{}'. We just get an
>> empty expression list. When one writes '{NULL, 0}', we get an implicit
>
> I would vote for dropping the part that checks for implicit cats so that
> we unify all code on {} for sentinels. I guess that all we would have to
> do is to drop the loop and only check if the list is empty.

Yes, indeed, it probably simplifies things a lot more than is
immediately obvious as well. I will reroll.

>
> Other than that the code looks good to me:
>
> Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
diff mbox series

Patch

diff --git a/tools/sparse/sparse-ltp.c b/tools/sparse/sparse-ltp.c
index b1677d336..078d3eb43 100644
--- a/tools/sparse/sparse-ltp.c
+++ b/tools/sparse/sparse-ltp.c
@@ -126,10 +126,102 @@  static void check_symbol_visibility(const struct symbol *const sym)
 		name);
 }
 
+/* See base_type() in dissect.c */
+static struct symbol *unwrap_base_type(const struct symbol *sym)
+{
+	switch (sym->ctype.base_type->type) {
+	case SYM_ARRAY:
+	case SYM_NODE:
+	case SYM_PTR:
+		return unwrap_base_type(sym->ctype.base_type);
+	default:
+		return sym->ctype.base_type;
+	}
+}
+
+/* Checks if some struct array initializer is terminated with a zeroed
+ * (NULL) item e.g. {} or {NULL, NULL, ...}
+ */
+static int is_terminated_with_null_struct(const struct symbol *const sym)
+{
+	struct expression *arr_init = sym->initializer;
+	struct expression *item_init =
+		last_ptr_list((struct ptr_list *)arr_init->expr_list);
+	struct expression *entry;
+
+	if (item_init->type == EXPR_POS)
+		item_init = item_init->init_expr;
+
+	FOR_EACH_PTR(item_init->expr_list, entry) {
+		if (entry->init_expr->type == EXPR_IMPLIED_CAST)
+			continue;
+
+		return 0;
+	} END_FOR_EACH_PTR(entry);
+
+	return 1;
+}
+
+/* Check for (one instance of) LTP-005
+ *
+ * The tags array is only accessed when the test fails. So we perform
+ * a static check to ensure it ends with {} or {NULL, NULL}.
+ */
+static void check_tag_initializer(const struct symbol *const sym)
+{
+	if (is_terminated_with_null_struct(sym))
+		return;
+
+	warning(sym->pos,
+		"test.tags array doesn't appear to be null-terminated; did you forget to add '{}' as the final entry?");
+}
+
+/* Find struct tst_test test = { ... } and perform tests on its initializer */
+static void check_test_struct(const struct symbol *const sym)
+{
+	static struct ident *tst_test, *tst_test_test, *tst_tag;
+	struct ident *ctype_name = NULL;
+	struct expression *init = sym->initializer;
+	struct expression *entry;
+
+	if (!sym->ctype.base_type)
+		return;
+
+	ctype_name = sym->ctype.base_type->ident;
+
+	if (!init)
+		return;
+
+	if (!tst_test_test) {
+		tst_test = built_in_ident("tst_test");
+		tst_test_test = built_in_ident("test");
+		tst_tag = built_in_ident("tst_tag");
+	}
+
+	if (sym->ident != tst_test_test)
+		return;
+
+	if (ctype_name != tst_test)
+		return;
+
+	FOR_EACH_PTR(init->expr_list, entry) {
+		if (entry->init_expr->type != EXPR_SYMBOL)
+			continue;
+
+		const struct symbol *entry_init = entry->init_expr->symbol;
+		const struct symbol *entry_ctype = unwrap_base_type(entry_init);
+
+		if (entry_ctype->ident == tst_tag)
+			check_tag_initializer(entry_init);
+	} END_FOR_EACH_PTR(entry);
+
+}
+
 /* AST level checks */
 static void do_symbol_checks(struct symbol *sym)
 {
 	check_symbol_visibility(sym);
+	check_test_struct(sym);
 }
 
 /* Compile the AST into a graph of basicblocks */