[PATCH 0/8] format check tweaks

[PATCH 0/8] format check tweaks

[Luc Van Oostenryck]

Ben, these are small changes I think should be applied with
your series, on top of the patches I send yesterday.
I've pushed everything at:
	git://github.com/lucvoo/sparse-dev.git format-check
If you're fine with these, I can squeeze them with the series.

There are also a few tests I don't agree with:
	const void *ptr = ...
	printf("%s", ptr);
These tests silently accept this, but they should warn.
But this can be fixed at a later step.


Luc Van Oostenryck (8):
  need to strip SYM_NODE before comparing types
  add helper get_nth_expression()
  move the definition of FMT_{PRINTF,SCANF}
  parse format attribute less verbosely
  call verify_format_attribute() unconditionally
  s/data/type/ for struct format_type
  add support for "%ls"
  add support for "%Lx" (and "%Ls")

 evaluate.c                           |  4 +--
 lib.h                                |  5 +++
 parse.c                              | 30 +++++++----------
 symbol.h                             |  7 ++++
 validation/varargs-format-dyn-prec.c | 11 ++++++
 verify-format.c                      | 50 ++++++++++++++++------------
 6 files changed, 65 insertions(+), 42 deletions(-)
 create mode 100644 validation/varargs-format-dyn-prec.c

-- 
2.28.0

[PATCH 1/8] need to strip SYM_NODE before comparing types

[Luc Van Oostenryck]

Every variable has a 'symbol type' of SYM_NODE which needs to
be stripped before comparing with builtin types like int_ctype.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 validation/varargs-format-dyn-prec.c | 11 +++++++++++
 verify-format.c                      |  3 +++
 2 files changed, 14 insertions(+)
 create mode 100644 validation/varargs-format-dyn-prec.c

diff --git a/validation/varargs-format-dyn-prec.c b/validation/varargs-format-dyn-prec.c
new file mode 100644
index 000000000000..a143f1661c84
--- /dev/null
+++ b/validation/varargs-format-dyn-prec.c
@@ -0,0 +1,11 @@
+extern void pf(const char *msg, ...) __attribute__((format(printf, 1, 2)));
+
+static void test(int prec)
+{
+	pf("%*s\n", prec, "xyz");
+}
+
+/*
+ * check-name: variadic formatting test dynamic precision
+ * check-command: sparse -Wformat $file
+ */
diff --git a/verify-format.c b/verify-format.c
index 2da9d2069121..ba6cb5646dba 100644
--- a/verify-format.c
+++ b/verify-format.c
@@ -325,6 +325,9 @@ static int parse_format_printf_argfield(const char **fmtptr,
 	if (ctype) {
 		struct symbol *target = &int_ctype;
 
+		if (ctype->type == SYM_NODE)
+			ctype = ctype->ctype.base_type;
+
 		if (ctype != &int_ctype && ctype != &uint_ctype) {
 			warning(expr->pos, "incorrect type for %s argument %d", which, argpos);
 			info(expr->pos, "   expected %s", show_typename(target));
-- 
2.28.0

[PATCH 2/8] add helper get_nth_expression()

[Luc Van Oostenryck]

This will be used for -Wformat.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 lib.h           | 5 +++++
 verify-format.c | 5 -----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/lib.h b/lib.h
index b35debc83288..957586dbd80a 100644
--- a/lib.h
+++ b/lib.h
@@ -199,6 +199,11 @@ static inline struct expression *first_expression(struct expression_list *head)
 	return first_ptr_list((struct ptr_list *)head);
 }
 
+static inline struct expression *get_nth_expression(struct expression_list *head, unsigned int n)
+{
+	return ptr_list_nth_entry((struct ptr_list *)head, n);
+}
+
 static inline pseudo_t first_pseudo(struct pseudo_list *head)
 {
 	return first_ptr_list((struct ptr_list *)head);
diff --git a/verify-format.c b/verify-format.c
index ba6cb5646dba..939605f55ef5 100644
--- a/verify-format.c
+++ b/verify-format.c
@@ -113,11 +113,6 @@ static struct format_type printf_fmt_ptr_ref = {
 	.test = printf_fmt_pointer,
 };
 
-static struct expression *get_nth_expression(struct expression_list *args, int nr)
-{
-	return ptr_list_nth_entry((struct ptr_list *)args, nr);
-}
-
 static int is_float_spec(char t)
 {
 	return t == 'f' || t == 'g' || t == 'F' || t == 'G';
-- 
2.28.0

[PATCH 3/8] move the definition of FMT_{PRINTF,SCANF}

[Luc Van Oostenryck]

Move these from parse.c to symbol.h so that they can be reused
when verifying the format.

Also, add a definition for unknown format type: FMT_UNKNOWN

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 parse.c  | 6 ------
 symbol.h | 7 +++++++
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/parse.c b/parse.c
index 1b021b87549e..c2d29318149f 100644
--- a/parse.c
+++ b/parse.c
@@ -120,12 +120,6 @@ static void asm_modifier(struct token *token, unsigned long *mods, unsigned long
 	*mods |= mod;
 }
 
-/* the types of formatting from __attribute__((format)) */
-enum {
-	FMT_PRINTF = 0,
-	FMT_SCANF,
-};
-
 static struct symbol_op typedef_op = {
 	.type = KW_MODIFIER,
 	.declarator = storage_specifier,
diff --git a/symbol.h b/symbol.h
index 55c7e3330ec3..0d5439ee93f7 100644
--- a/symbol.h
+++ b/symbol.h
@@ -95,6 +95,13 @@ extern struct context *alloc_context(void);
 
 DECLARE_PTR_LIST(context_list, struct context);
 
+/* the types of formatting from __attribute__((format)) */
+enum {
+	FMT_UNKNOWN,
+	FMT_PRINTF,
+	FMT_SCANF,
+};
+
 struct attr_format {
 	unsigned short index;	/* index in argument list for format string */
 	unsigned short first;	/* where first variadic argument is */
-- 
2.28.0

[PATCH 4/8] parse format attribute less verbosely

[Luc Van Oostenryck]

The error handling in the parsing of the format attribute is a bit too
verbose and to strict:
*) the message "only printf format attribute supported" is just noise
   for any file using 'format(scanf, ...)' when -Wformat is set
   and is not needed if -Wformat is disabled, so remove it.
*) the message "incorrect format attribute" is not needed because
   any parsing error will already be reported.
OTOH, it is useful to check that the first argument is an identifier,
so check this.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 parse.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/parse.c b/parse.c
index c2d29318149f..55cca01ec381 100644
--- a/parse.c
+++ b/parse.c
@@ -1215,17 +1215,18 @@ static int invalid_format_args(long long start, long long at)
 static struct token *attribute_format(struct token *token, struct symbol *attr, struct decl_state *ctx)
 {
 	struct expression *args[3];
-	struct symbol *fmt_sym = NULL;
+	int type = FMT_UNKNOWN;
 
 	/* expecting format ( type, start, va_args at) */
 
 	token = expect(token, '(', "after format attribute");
-	if (token_type(token) == TOKEN_IDENT)
-		fmt_sym = lookup_keyword(token->ident, NS_KEYWORD);
-	if (fmt_sym)
-		if (!fmt_sym->op || fmt_sym->op->type != KW_FORMAT)
-			fmt_sym = NULL;
-
+	if (token_type(token) != TOKEN_IDENT) {
+		sparse_error(token->pos, "identifier expected for format type");
+	} else {
+		struct symbol *sym = lookup_keyword(token->ident, NS_KEYWORD);
+		if (sym && sym->op && sym->op->type == KW_FORMAT)
+			type = sym->op->class;
+	}
 	token = conditional_expression(token, &args[0]);
 	token = expect(token, ',', "format attribute type");
 	token = conditional_expression(token, &args[1]);
@@ -1233,11 +1234,10 @@ static struct token *attribute_format(struct token *token, struct symbol *attr,
 	token = conditional_expression(token, &args[2]);
 	token = expect(token, ')', "format attribute arg position");
 
-	if (!fmt_sym || !args[0] || !args[1] || !args[2]) {
-		warning(token->pos, "incorrect format attribute");
-	} else if (fmt_sym->op->class != FMT_PRINTF) {
-		/* skip anything that isn't printf for the moment */
-		warning(token->pos, "only printf format attribute supported");
+	if (!args[0] || !args[1] || !args[2]) {
+		// incorrect format attribute
+	} else if (type != FMT_PRINTF) {
+		// only printf-style is supported, skip anything else
 	} else {
 		long long start, at;
 
-- 
2.28.0

[PATCH 5/8] call verify_format_attribute() unconditionally

[Luc Van Oostenryck]

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 evaluate.c      | 4 +---
 verify-format.c | 2 ++
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/evaluate.c b/evaluate.c
index fb3c0adb2220..35ced8cb6fb5 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -2388,9 +2388,7 @@ int evaluate_arguments(struct symbol *fn, struct symbol_list *argtypes,
 	} END_FOR_EACH_PTR(expr);
 	FINISH_PTR_LIST(argtype);
 
-	if (fn && Wformat)
-		verify_format_attribute(fn, head);
-
+	verify_format_attribute(fn, head);
 	return 1;
 }
 
diff --git a/verify-format.c b/verify-format.c
index 939605f55ef5..2eaba6653686 100644
--- a/verify-format.c
+++ b/verify-format.c
@@ -452,6 +452,8 @@ void verify_format_attribute(struct symbol *fn, struct expression_list *args)
 	struct expression *init;
 	const char *fmt_string;
 
+	if (!fn || !Wformat)
+		return;
 	if (!fn->ctype.format.index)
 		return;
 
-- 
2.28.0

[PATCH 6/8] s/data/type/ for struct format_type

[Luc Van Oostenryck]

The name of the field 'data' in struct format_type seems to indicate
that it can contain arbitrary untyped stuff but it's a 'struct symbol'
pointer and always contains the expected type of the argument.

So use a more specific name for it 'type'.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 verify-format.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/verify-format.c b/verify-format.c
index 2eaba6653686..95ff524c03cf 100644
--- a/verify-format.c
+++ b/verify-format.c
@@ -51,7 +51,7 @@ struct format_type {
 				struct symbol *ctype,
 				struct symbol **target,
 				const char **typediff);
-	struct symbol	*data;
+	struct symbol	*type;
 };
 
 struct format_state {
@@ -67,7 +67,7 @@ static int printf_fmt_numtype(struct format_type *fmt,
 			      struct symbol *ctype,
 			      struct symbol **target, const char **typediff)
 {
-	struct symbol *type = fmt->data;
+	struct symbol *type = fmt->type;
 	*target = type;
 	return check_assignment_types(*target, expr, typediff);
 }
@@ -133,7 +133,7 @@ static struct format_type *parse_printf_get_fmt(struct format_type *type,
 	} else if (*ptr == 'c') {
 		ptr++;
 		type->test = printf_fmt_numtype;
-		type->data = &char_ctype;
+		type->type = &char_ctype;
 	} else if (*ptr == 'p') {
 		ptr++;
 		type->test = printf_fmt_print_pointer;
@@ -152,12 +152,12 @@ static struct format_type *parse_printf_get_fmt(struct format_type *type,
 		if (*ptr == 'd' || *ptr == 'i') {
 			ptr++;
 			type->test = printf_fmt_numtype;
-			type->data = ssize_t_ctype;
+			type->type = ssize_t_ctype;
 		} else if (*ptr == 'u' || *ptr == 'x' || *ptr == 'X' ||
 			   *ptr == 'o') {
 			ptr++;
 			type->test = printf_fmt_numtype;
-			type->data = size_t_ctype;
+			type->type = size_t_ctype;
 		}
 	} else {
 		if (*ptr == 'l') {
@@ -190,16 +190,16 @@ static struct format_type *parse_printf_get_fmt(struct format_type *type,
 			type->test = printf_fmt_numtype;
 			switch (szmod) {
 			case -1:
-				type->data = &ushort_ctype;
+				type->type = &ushort_ctype;
 				break;
 			case 0:
-				type->data = &uint_ctype;
+				type->type = &uint_ctype;
 				break;
 			case 1:
-				type->data = &ulong_ctype;
+				type->type = &ulong_ctype;
 				break;
 			case 2:
-				type->data = &ullong_ctype;
+				type->type = &ullong_ctype;
 				break;
 			default:
 				type->test = NULL;
@@ -209,27 +209,27 @@ static struct format_type *parse_printf_get_fmt(struct format_type *type,
 			type->test = printf_fmt_numtype;
 			switch (szmod) {
 			case -1:
-				type->data = &short_ctype;
+				type->type = &short_ctype;
 				break;
 			case 0:
-				type->data = &int_ctype;
+				type->type = &int_ctype;
 				break;
 			case 1:
-				type->data = &long_ctype;
+				type->type = &long_ctype;
 				break;
 			case 2:
-				type->data = &llong_ctype;
+				type->type = &llong_ctype;
 				break;
 			default:
 				type->test = NULL;
 			}
 		} else if (*ptr == 'L' && is_float_spec(ptr[1])) {
 			type->test = printf_fmt_numtype;
-			type->data = &ldouble_ctype;
+			type->type = &ldouble_ctype;
 			ptr += 2;
 		} else if (is_float_spec(*ptr)) {
 			type->test = printf_fmt_numtype;
-			type->data = szmod == 1 ? &ldouble_ctype :  &double_ctype;
+			type->type = szmod == 1 ? &ldouble_ctype :  &double_ctype;
 			ptr++;
 		} else if (*ptr == 'n') {	/* pointer to an de-referenced int/etc */
 			// todo - we should construct pointer to int/etc //
-- 
2.28.0

[PATCH 7/8] add support for "%ls"

[Luc Van Oostenryck]

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 verify-format.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/verify-format.c b/verify-format.c
index 95ff524c03cf..b27440b87c6b 100644
--- a/verify-format.c
+++ b/verify-format.c
@@ -77,7 +77,7 @@ static int printf_fmt_string(struct format_type *fmt,
 			     struct symbol *ctype,
 			     struct symbol **target, const char **typediff)
 {
-	*target = &const_string_ctype;
+	*target = fmt->type;
 	return check_assignment_types(*target, expr, typediff);
 }
 
@@ -130,6 +130,7 @@ static struct format_type *parse_printf_get_fmt(struct format_type *type,
 	if (*ptr == 's') {
 		ptr++;
 		type->test = printf_fmt_string;
+		type->type = &const_string_ctype;
 	} else if (*ptr == 'c') {
 		ptr++;
 		type->test = printf_fmt_numtype;
@@ -231,6 +232,10 @@ static struct format_type *parse_printf_get_fmt(struct format_type *type,
 			type->test = printf_fmt_numtype;
 			type->type = szmod == 1 ? &ldouble_ctype :  &double_ctype;
 			ptr++;
+		} else if (*ptr == 's') {
+			type->test = printf_fmt_string;
+			type->type = &const_wstring_ctype;
+			ptr++;
 		} else if (*ptr == 'n') {	/* pointer to an de-referenced int/etc */
 			// todo - we should construct pointer to int/etc //
 			// also should not have any flags or widths for this
-- 
2.28.0

[PATCH 8/8] add support for "%Lx" (and "%Ls")

[Luc Van Oostenryck]

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 verify-format.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/verify-format.c b/verify-format.c
index b27440b87c6b..4b4730285237 100644
--- a/verify-format.c
+++ b/verify-format.c
@@ -168,6 +168,9 @@ static struct format_type *parse_printf_get_fmt(struct format_type *type,
 				szmod++;
 				ptr++;
 			}
+		} else if (*ptr == 'L') {
+			szmod++;
+			ptr++;
 		} else {
 			if (*ptr == 'h') { // short/char to int
 				szmod = -1;
-- 
2.28.0

Re: [PATCH 0/8] format check tweaks

[Ben Dooks]

On 05/10/2020 02:59, Luc Van Oostenryck wrote:
> Ben, these are small changes I think should be applied with
> your series, on top of the patches I send yesterday.
> I've pushed everything at:
> 	git://github.com/lucvoo/sparse-dev.git format-check
> If you're fine with these, I can squeeze them with the series.
> 
> There are also a few tests I don't agree with:
> 	const void *ptr = ...
> 	printf("%s", ptr);
> These tests silently accept this, but they should warn.
> But this can be fixed at a later step.

ok, thanks.

I'm going to try and work out the best way to deal with the kernel
extra funsies. I have a few ideas but yet to make a coherent document
about them.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html

Re: [PATCH 0/8] format check tweaks

[Luc Van Oostenryck]

On Mon, Oct 05, 2020 at 10:04:43AM +0100, Ben Dooks wrote:
> On 05/10/2020 02:59, Luc Van Oostenryck wrote:
> > Ben, these are small changes I think should be applied with
> > your series, on top of the patches I send yesterday.
> > I've pushed everything at:
> > 	git://github.com/lucvoo/sparse-dev.git format-check
> > If you're fine with these, I can squeeze them with the series.
> > 
> > There are also a few tests I don't agree with:
> > 	const void *ptr = ...
> > 	printf("%s", ptr);
> > These tests silently accept this, but they should warn.
> > But this can be fixed at a later step.
> 
> ok, thanks.

Hi,

I've now pushed it on a separate branch on the official tree:
  git://git.kernel.org/pub/scm/devel/sparse/sparse.git format-check

It will thus not be rebased anymore and any changes will need to be
incrementally on top of it.

> I'm going to try and work out the best way to deal with the kernel
> extra funsies. I have a few ideas but yet to make a coherent document
> about them.

Well, the reason I've not yet merged it with the main branch is
because enabling -Wformat creates really a lot of warnings in
the kernel and people definitively use this.

Most of these warnings (if not all) is caused by using
check_assignment_types() which was good as a quick & dirty solution
in the early stages but isn't at all adequate as a true solution
and this for several reasons. I've also begin to take a look at
this with a relatively satisfying result by adopting the following
strategy:
	Since no 'checking' function in evaluate.c has the needed
	characteristics, simplest is to create in verify-format.c
	what is needed.
This also gives the flexibility needed to support things like
-Wformat-signedness.

I add here below only small extract because it's for now quite
messy and need quite a bit of polishing.

-- Luc


enum {
	CLASS_OTHER,
	CLASS_INT,
	CLASS_BITWISE,
	CLASS_FLOAT,
	CLASS_PTR,
};

///
// retrieve a 'type class' for quick testing and extract the base type
static int type_class(struct symbol *type, struct symbol **base)
{
	if (type->type == SYM_NODE)
		type = type->ctype.base_type;
	if (type->type == SYM_ENUM)
		type = type->ctype.base_type;
	*base = type;
	if (type->type == SYM_BASETYPE) {
		struct symbol *kind = type->ctype.base_type;
		if (kind == &int_type)
			return CLASS_INT;
		if (kind == &fp_type)
			return CLASS_FLOAT;
	}
	if (type->type == SYM_PTR)
		return CLASS_PTR;
	if (type->type == SYM_RESTRICT)
		return CLASS_BITWISE;
	return CLASS_OTHER;
}

static const char *printf_fmt_flt(struct format_type *fmt, struct symbol *source)
{
	const char *typediff = "different base types";
	struct symbol *target = fmt->type;
	struct symbol *base;

	if (type_class(source, &base) != CLASS_FLOAT)
		return typediff;
	if (base == target)
		return NULL;
	return typediff;
}

static const char *printf_fmt_string(struct format_type *fmt, struct symbol *source)
{
	const char *typediff = "different base types";
	struct symbol *target = fmt->type;

	examine_pointer_target(target);
	examine_pointer_target(source);

	if (type_class(source, &source) != CLASS_PTR)
		return typediff;
	if (source->ctype.as)
		return "different address spaces";

	// get the base type
	type_class(source->ctype.base_type, &source);
	type_class(source, &source);

	if (target == &const_string_ctype) {
		if (source == &char_ctype)
			return NULL;
		if (source == &uchar_ctype)
			return NULL;
		if (source == &schar_ctype)
			return NULL;
	} if (target == &const_wstring_ctype) {
		if (source == wchar_ctype)
			return NULL;
	}
	return typediff;
}

Re: [PATCH 0/8] format check tweaks

[Ben Dooks]

On 06/10/2020 00:47, Luc Van Oostenryck wrote:
> On Mon, Oct 05, 2020 at 10:04:43AM +0100, Ben Dooks wrote:
>> On 05/10/2020 02:59, Luc Van Oostenryck wrote:
>>> Ben, these are small changes I think should be applied with
>>> your series, on top of the patches I send yesterday.
>>> I've pushed everything at:
>>> 	git://github.com/lucvoo/sparse-dev.git format-check
>>> If you're fine with these, I can squeeze them with the series.
>>>
>>> There are also a few tests I don't agree with:
>>> 	const void *ptr = ...
>>> 	printf("%s", ptr);
>>> These tests silently accept this, but they should warn.
>>> But this can be fixed at a later step.
>>
>> ok, thanks.
> 
> Hi,
> 
> I've now pushed it on a separate branch on the official tree:
>    git://git.kernel.org/pub/scm/devel/sparse/sparse.git format-check
> 
> It will thus not be rebased anymore and any changes will need to be
> incrementally on top of it.
> 
>> I'm going to try and work out the best way to deal with the kernel
>> extra funsies. I have a few ideas but yet to make a coherent document
>> about them.
> 
> Well, the reason I've not yet merged it with the main branch is
> because enabling -Wformat creates really a lot of warnings in
> the kernel and people definitively use this.
> 
> Most of these warnings (if not all) is caused by using
> check_assignment_types() which was good as a quick & dirty solution
> in the early stages but isn't at all adequate as a true solution
> and this for several reasons. I've also begin to take a look at
> this with a relatively satisfying result by adopting the following
> strategy:
> 	Since no 'checking' function in evaluate.c has the needed
> 	characteristics, simplest is to create in verify-format.c
> 	what is needed.
> This also gives the flexibility needed to support things like
> -Wformat-signedness.
> 
> I add here below only small extract because it's for now quite
> messy and need quite a bit of polishing.

I've not had time to run the latest against the kernel as I forgot
the disc password to my main machine and have had to rebuild the
system from scratch.

Let me know if there's anything else I can help with.



-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html

Re: [PATCH 0/8] format check tweaks

[Luc Van Oostenryck]

On Sun, Oct 11, 2020 at 08:51:59PM +0100, Ben Dooks wrote:
> 
> I've not had time to run the latest against the kernel as I forgot
> the disc password to my main machine and have had to rebuild the
> system from scratch.

Definitively not fun :(
 
> Let me know if there's anything else I can help with.

Better wait till I've something more or less complete, probably tomorrow.

-- Luc