[LTP] [PATCH v2 1/3] doc: Add LTP-005: Array must be sentinel terminated

[LTP] [PATCH v2 1/3] doc: Add LTP-005: Array must be sentinel terminated

[Richard Palethorpe via ltp]

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
Reviewed-by: Petr Vorel <pvorel@suse.cz>
Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
---

V2:
* Don't allow {NULL, NULL}, {0}, etc.
* The above resulted in two errors
  being detected which are fixed in patch 3

 doc/rules.tsv | 1 +
 1 file changed, 1 insertion(+)

diff --git a/doc/rules.tsv b/doc/rules.tsv
index 2dfeca9f9..66dbdeccf 100644
--- a/doc/rules.tsv
+++ b/doc/rules.tsv
@@ -3,3 +3,4 @@ LTP-001	Library source files have tst_ prefix
 LTP-002	TST_RET and TST_ERR are never modified by test library functions
 LTP-003 Externally visible library symbols have the tst_ prefix
 LTP-004 Test executable symbols are marked static
+LTP-005 Array must terminate with a sentinel value (i.e. NULL or '{}')
-- 
2.34.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

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

[Richard Palethorpe via ltp]

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', we get an implicit cast
to '$a_type *'. Currently we only handle '{}' as this is what
test.tags requires. Using '{NULL, NULL}' is semantically equivalent to
'{}', but it simplifies things to only accept '{}'.

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>
Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
Reviewed-by: Petr Vorel <pvorel@suse.cz>
---
 tools/sparse/sparse-ltp.c | 84 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 84 insertions(+)

diff --git a/tools/sparse/sparse-ltp.c b/tools/sparse/sparse-ltp.c
index b1677d336..3a38229f1 100644
--- a/tools/sparse/sparse-ltp.c
+++ b/tools/sparse/sparse-ltp.c
@@ -126,10 +126,94 @@ 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 blank
+ * (zeroed) item i.e. {}
+ */
+static bool is_terminated_with_null_struct(const struct symbol *const sym)
+{
+	const struct expression *const arr_init = sym->initializer;
+	const struct expression *item_init =
+		last_ptr_list((struct ptr_list *)arr_init->expr_list);
+
+	if (item_init->type == EXPR_POS)
+		item_init = item_init->init_expr;
+
+	return ptr_list_empty((struct ptr_list *)item_init->expr_list);
+}
+
+/* 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 {}
+ */
+static void check_tag_initializer(const struct symbol *const sym)
+{
+	if (is_terminated_with_null_struct(sym))
+		return;
+
+	warning(sym->pos,
+		"LTP-005: 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 */
-- 
2.34.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

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

[Cyril Hrubis]

Hi!
> +/* Checks if some struct array initializer is terminated with a blank
> + * (zeroed) item i.e. {}
> + */
> +static bool is_terminated_with_null_struct(const struct symbol *const sym)
> +{
> +	const struct expression *const arr_init = sym->initializer;
> +	const struct expression *item_init =
> +		last_ptr_list((struct ptr_list *)arr_init->expr_list);
> +
> +	if (item_init->type == EXPR_POS)
> +		item_init = item_init->init_expr;
> +
> +	return ptr_list_empty((struct ptr_list *)item_init->expr_list);
> +}

Looks good now.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v2 3/3] getrusage03, setsockopt03: Add sentinel value to tags

[Richard Palethorpe via ltp]

LTP-005

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---
 testcases/kernel/syscalls/getrusage/getrusage03.c   | 1 +
 testcases/kernel/syscalls/setsockopt/setsockopt03.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/testcases/kernel/syscalls/getrusage/getrusage03.c b/testcases/kernel/syscalls/getrusage/getrusage03.c
index ef2f62a18..bf5127483 100644
--- a/testcases/kernel/syscalls/getrusage/getrusage03.c
+++ b/testcases/kernel/syscalls/getrusage/getrusage03.c
@@ -180,6 +180,7 @@ static struct tst_test test = {
 	.min_kver = "2.6.32",
 	.tags = (const struct tst_tag[]) {
 		{"linux-git", "1f10206cf8e9"},
+		{}
 	},
 	.test = run,
 	.tcnt = ARRAY_SIZE(testfunc_list),
diff --git a/testcases/kernel/syscalls/setsockopt/setsockopt03.c b/testcases/kernel/syscalls/setsockopt/setsockopt03.c
index b7a4447ba..191c4cdfe 100644
--- a/testcases/kernel/syscalls/setsockopt/setsockopt03.c
+++ b/testcases/kernel/syscalls/setsockopt/setsockopt03.c
@@ -81,5 +81,6 @@ static struct tst_test test = {
 	.tags = (const struct tst_tag[]){
 		{"linux-git", "ce683e5f9d04"},
 		{"CVE", "CVE-2016-4997"},
+		{}
 	}
 };
-- 
2.34.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2 3/3] getrusage03, setsockopt03: Add sentinel value to tags

[Cyril Hrubis]

Hi!
Looks obviously fine:

Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

Also good catch, this really shows that the automation is worth the
effort.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2 3/3] getrusage03, setsockopt03: Add sentinel value to tags

[Richard Palethorpe]

Hello,

Cyril Hrubis <chrubis@suse.cz> writes:

> Hi!
> Looks obviously fine:
>
> Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

Pushed! Thanks!

>
> Also good catch, this really shows that the automation is worth the
> effort.

I hope so and perhaps this can be generalised by creating an attribute
e.g. (null_terminated) which could be upstreamed to Sparse. Then
eventually it might be adopted by compilers.

-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp