[PATCH 0/5] check_kernel_printf stuff

[PATCH 0/5] check_kernel_printf stuff

[Rasmus Villemoes]

Hi Dan

Can I get you to look at the false positive issue in the last patch
before applying it? It's a very long time since I've done anything
with sparse/smatch, and I'm afraid I don't have the time to dig into
it (obviously, don't feel obliged; if it's not trivial then please
just drop the patch for now).

Tobin, you may be interested in this stuff, so you've been cc'ed.

Rasmus Villemoes (5):
  check_kernel_printf.c: use get_real_base_type in typedef_lookup
  check_kernel_printf.c: remove copy-pastoed variable
  check_kernel_printf.c: check %pOF argument is struct device_node*
  check_kernel_printf.c: prepare for more tests for integer specifiers
  check_kernel_printf.c: warn about "%lx", (long)ptr

 check_kernel_printf.c | 50 +++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 43 insertions(+), 7 deletions(-)

-- 
2.11.0

[PATCH 1/5] check_kernel_printf.c: use get_real_base_type in typedef_lookup

[Rasmus Villemoes]

Since gfp_t is a typedef to a __bitwise type, the type we currently
return is not the same as the basetype we compare to in flagstring()
(namely uint), giving confusing false positives.

Signed-off-by: Rasmus Villemoes <rv@rasmusvillemoes.dk>
---
 check_kernel_printf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/check_kernel_printf.c b/check_kernel_printf.c
index ed1fdd8c..715e7b42 100644
--- a/check_kernel_printf.c
+++ b/check_kernel_printf.c
@@ -336,7 +336,7 @@ static struct symbol *_typedef_lookup(const char *name)
 	node = lookup_symbol(id, NS_TYPEDEF);
 	if (!node || node->type != SYM_NODE)
 		return NULL;
-	return node->ctype.base_type;
+	return get_real_base_type(node);
 }
 
 static void typedef_lookup(struct typedef_lookup *tl)
-- 
2.11.0

[PATCH 2/5] check_kernel_printf.c: remove copy-pastoed variable

[Rasmus Villemoes]

Signed-off-by: Rasmus Villemoes <rv@rasmusvillemoes.dk>
---
 check_kernel_printf.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/check_kernel_printf.c b/check_kernel_printf.c
index 715e7b42..7bef7e7d 100644
--- a/check_kernel_printf.c
+++ b/check_kernel_printf.c
@@ -639,8 +639,6 @@ static void flag_string(const char *fmt, struct symbol *type, struct symbol *bas
 
 static void device_node_string(const char *fmt, struct symbol *type, struct symbol *basetype, int vaidx)
 {
-	static struct typedef_lookup gfp = { .name = "gfp_t" };
-
 	if (fmt[1] != 'F')
 		sm_msg("error: %%pO can only be followed by 'F'");
 }
-- 
2.11.0

[PATCH 3/5] check_kernel_printf.c: check %pOF argument is struct device_node*

[Rasmus Villemoes]

Signed-off-by: Rasmus Villemoes <rv@rasmusvillemoes.dk>
---
 check_kernel_printf.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/check_kernel_printf.c b/check_kernel_printf.c
index 7bef7e7d..fb8d3049 100644
--- a/check_kernel_printf.c
+++ b/check_kernel_printf.c
@@ -639,8 +639,13 @@ static void flag_string(const char *fmt, struct symbol *type, struct symbol *bas
 
 static void device_node_string(const char *fmt, struct symbol *type, struct symbol *basetype, int vaidx)
 {
-	if (fmt[1] != 'F')
+	if (fmt[1] != 'F') {
 		sm_msg("error: %%pO can only be followed by 'F'");
+		return;
+	}
+	if (!is_struct_tag(basetype, "device_node"))
+		sm_msg("error: '%%pOF' expects argument of type 'struct device_node*', argument %d has type '%s'",
+		       vaidx, type_to_str(type));
 }
 
 static void
-- 
2.11.0

[PATCH 4/5] check_kernel_printf.c: prepare for more tests for integer specifiers

[Rasmus Villemoes]

Signed-off-by: Rasmus Villemoes <rv@rasmusvillemoes.dk>
---
 check_kernel_printf.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/check_kernel_printf.c b/check_kernel_printf.c
index fb8d3049..87c6744a 100644
--- a/check_kernel_printf.c
+++ b/check_kernel_printf.c
@@ -1011,9 +1011,11 @@ do_check_printf_call(const char *caller, const char *name, struct expression *ca
 
 		if (spec.flags & SPECIAL && has_hex_prefix(orig_fmt, old_fmt))
 			sm_msg("warn: '%.2s' prefix is redundant when # flag is used", old_fmt-2);
-		if (is_integer_specifier(spec.type) && spec.base != 16 && has_hex_prefix(orig_fmt, old_fmt))
-			sm_msg("warn: '%.2s' prefix is confusing together with '%.*s' specifier",
-			       old_fmt-2, (int)(fmt-old_fmt), old_fmt);
+		if (is_integer_specifier(spec.type)) {
+			if (spec.base != 16 && has_hex_prefix(orig_fmt, old_fmt))
+				sm_msg("warn: '%.2s' prefix is confusing together with '%.*s' specifier",
+				       old_fmt-2, (int)(fmt-old_fmt), old_fmt);
+		}
 
 		switch (spec.type) {
 		/* case FORMAT_TYPE_NONE: */
-- 
2.11.0

[PATCH 5/5] check_kernel_printf.c: warn about "%lx", (long)ptr

[Rasmus Villemoes]

For some reason this spits out an enourmous amount of false positives,
making this entirely useless. We hit a lot of "%lx", (long)(a - b),
but I don't understand why the a-b expression (a pointer difference)
passes is_ptr_type().

Signed-off-by: Rasmus Villemoes <rv@rasmusvillemoes.dk>
---
 check_kernel_printf.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/check_kernel_printf.c b/check_kernel_printf.c
index 87c6744a..b4a9dcde 100644
--- a/check_kernel_printf.c
+++ b/check_kernel_printf.c
@@ -939,6 +939,35 @@ static bool is_integer_specifier(int type)
 	}
 }
 
+static int
+is_cast_expr(struct expression *expr)
+{
+	switch (expr->type) {
+	case EXPR_CAST:
+	case EXPR_FORCE_CAST:
+		/* not EXPR_IMPLIED_CAST for our purposes */
+		return 1;
+	default:
+		return 0;
+	}
+}
+
+static void
+check_cast_from_pointer(const char *fmt, int len, struct expression *arg, int va_idx)
+{
+	/*
+	 * This can easily be fooled by passing 0+(long)ptr or doing
+	 * "long local_var = (long)ptr" and passing local_var to
+	 * %lx. Tough.
+	 */
+	if (!is_cast_expr(arg))
+		return;
+	while (is_cast_expr(arg))
+		arg = arg->cast_expression;
+	if (is_ptr_type(get_type(arg)))
+		sm_msg("warn: argument %d to %.*s specifier is cast from pointer",
+			va_idx, len, fmt);
+}
 
 static void
 do_check_printf_call(const char *caller, const char *name, struct expression *callexpr, struct expression *fmtexpr, int vaidx)
@@ -1015,6 +1044,8 @@ do_check_printf_call(const char *caller, const char *name, struct expression *ca
 			if (spec.base != 16 && has_hex_prefix(orig_fmt, old_fmt))
 				sm_msg("warn: '%.2s' prefix is confusing together with '%.*s' specifier",
 				       old_fmt-2, (int)(fmt-old_fmt), old_fmt);
+
+			check_cast_from_pointer(old_fmt, read, arg, vaidx);
 		}
 
 		switch (spec.type) {
-- 
2.11.0

Re: [PATCH 5/5] check_kernel_printf.c: warn about "%lx", (long)ptr

[Dan Carpenter]

On Wed, Oct 25, 2017 at 09:25:15PM +0200, Rasmus Villemoes wrote:
> For some reason this spits out an enourmous amount of false positives,
> making this entirely useless. We hit a lot of "%lx", (long)(a - b),
> but I don't understand why the a-b expression (a pointer difference)
> passes is_ptr_type().

Well, it is a pointer type.  If you do pointer math, you get pointer
results.  You can't really treat subtract different from addition
because container_of() is a subtraction.

regards,
dan carpenter

Re: [PATCH 5/5] check_kernel_printf.c: warn about "%lx", (long)ptr

[Rasmus Villemoes]

On 26 October 2017 at 12:48, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Wed, Oct 25, 2017 at 09:25:15PM +0200, Rasmus Villemoes wrote:
>> For some reason this spits out an enourmous amount of false positives,
>> making this entirely useless. We hit a lot of "%lx", (long)(a - b),
>> but I don't understand why the a-b expression (a pointer difference)
>> passes is_ptr_type().
>
> Well, it is a pointer type.  If you do pointer math, you get pointer
> results.  You can't really treat subtract different from addition
> because container_of() is a subtraction.

Huh? When I subtract one pointer from another, I get an _integer_. The
math done in container_of is subtracting an integer from a pointer
which does give a pointer, of course, but that's not the issue here.
The false positives I'm talking about are (at least those I checked)
something like

ptr = array;
<misc logic, incrementing ptr along the way>
if (uhoh)
  printk("trouble with array element %lx\n", (long)(ptr - array));

which is obviously ok.

Re: [PATCH 5/5] check_kernel_printf.c: warn about "%lx", (long)ptr

[Dan Carpenter]

On Thu, Oct 26, 2017 at 12:54:20PM +0200, Rasmus Villemoes wrote:
> On 26 October 2017 at 12:48, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > On Wed, Oct 25, 2017 at 09:25:15PM +0200, Rasmus Villemoes wrote:
> >> For some reason this spits out an enourmous amount of false positives,
> >> making this entirely useless. We hit a lot of "%lx", (long)(a - b),
> >> but I don't understand why the a-b expression (a pointer difference)
> >> passes is_ptr_type().
> >
> > Well, it is a pointer type.  If you do pointer math, you get pointer
> > results.  You can't really treat subtract different from addition
> > because container_of() is a subtraction.
> 
> Huh? When I subtract one pointer from another, I get an _integer_.

No.  You still get a pointer.  :P  That's just how type promotion works
in C.

> The math done in container_of is subtracting an integer from a pointer
> which does give a pointer, of course, but that's not the issue here.
> The false positives I'm talking about are (at least those I checked)
> something like
> 
> ptr = array;
> <misc logic, incrementing ptr along the way>
> if (uhoh)
>   printk("trouble with array element %lx\n", (long)(ptr - array));
> 
> which is obviously ok.

Right, yes.  You'll need to filter those subtractions:

static bool is_subtraction(struct expression *expr)
{
	sval_t sval;

	expr = strip_expr(expr);
	if (expr->type != EXPR_BINOP || expr->op != '-')
		return false;

	if (get_implied_value(expr->right, &sval))  /* constants */
		return false;

	return true;
}

regards,
dan carpenter

Re: [PATCH 5/5] check_kernel_printf.c: warn about "%lx", (long)ptr

[Dan Carpenter]

On Thu, Oct 26, 2017 at 02:10:19PM +0300, Dan Carpenter wrote:
> On Thu, Oct 26, 2017 at 12:54:20PM +0200, Rasmus Villemoes wrote:
> > On 26 October 2017 at 12:48, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > On Wed, Oct 25, 2017 at 09:25:15PM +0200, Rasmus Villemoes wrote:
> > >> For some reason this spits out an enourmous amount of false positives,
> > >> making this entirely useless. We hit a lot of "%lx", (long)(a - b),
> > >> but I don't understand why the a-b expression (a pointer difference)
> > >> passes is_ptr_type().
> > >
> > > Well, it is a pointer type.  If you do pointer math, you get pointer
> > > results.  You can't really treat subtract different from addition
> > > because container_of() is a subtraction.
> > 
> > Huh? When I subtract one pointer from another, I get an _integer_.
> 
> No.  You still get a pointer.  :P  That's just how type promotion works
> in C.
> 

Wait...  Maybe I'm wrong.

Let me check how that works.

regards,
dan carpenter

Re: [PATCH 5/5] check_kernel_printf.c: warn about "%lx", (long)ptr

[Rasmus Villemoes]

On 26 October 2017 at 13:10, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Thu, Oct 26, 2017 at 12:54:20PM +0200, Rasmus Villemoes wrote:
>> On 26 October 2017 at 12:48, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>> > On Wed, Oct 25, 2017 at 09:25:15PM +0200, Rasmus Villemoes wrote:
>> >> For some reason this spits out an enourmous amount of false positives,
>> >> making this entirely useless. We hit a lot of "%lx", (long)(a - b),
>> >> but I don't understand why the a-b expression (a pointer difference)
>> >> passes is_ptr_type().
>> >
>> > Well, it is a pointer type.  If you do pointer math, you get pointer
>> > results.  You can't really treat subtract different from addition
>> > because container_of() is a subtraction.
>>
>> Huh? When I subtract one pointer from another, I get an _integer_.
>
> No.  You still get a pointer.  :P  That's just how type promotion works
> in C.

C99, 6.5.6:
9 When two pointers are subtracted, both shall point to elements of
the same array object,
or one past the last element of the array object; the result is the
difference of the
subscripts of the two array elements. The size of the result is
implementation-defined,
and its type (a signed integer type) is ptrdiff_t defined in the
<stddef.h> header.

If sparse behaves differently, it's a bug in sparse. But it may well
just be me that don't quite understand the various helpers for
examining type, or how types are actually implemented in sparse. I see
that evaluate_ptr_sub does expr->ctype = ssize_t_ctype and return
ssize_t_ctype, so something else must be going on.

Re: [PATCH 5/5] check_kernel_printf.c: warn about "%lx", (long)ptr

[Dan Carpenter]

On Thu, Oct 26, 2017 at 01:23:42PM +0200, Rasmus Villemoes wrote:
> On 26 October 2017 at 13:10, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > On Thu, Oct 26, 2017 at 12:54:20PM +0200, Rasmus Villemoes wrote:
> >> On 26 October 2017 at 12:48, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >> > On Wed, Oct 25, 2017 at 09:25:15PM +0200, Rasmus Villemoes wrote:
> >> >> For some reason this spits out an enourmous amount of false positives,
> >> >> making this entirely useless. We hit a lot of "%lx", (long)(a - b),
> >> >> but I don't understand why the a-b expression (a pointer difference)
> >> >> passes is_ptr_type().
> >> >
> >> > Well, it is a pointer type.  If you do pointer math, you get pointer
> >> > results.  You can't really treat subtract different from addition
> >> > because container_of() is a subtraction.
> >>
> >> Huh? When I subtract one pointer from another, I get an _integer_.
> >
> > No.  You still get a pointer.  :P  That's just how type promotion works
> > in C.
> 
> C99, 6.5.6:
> 9 When two pointers are subtracted, both shall point to elements of
> the same array object,
> or one past the last element of the array object; the result is the
> difference of the
> subscripts of the two array elements. The size of the result is
> implementation-defined,
> and its type (a signed integer type) is ptrdiff_t defined in the
> <stddef.h> header.
> 
> If sparse behaves differently, it's a bug in sparse. But it may well
> just be me that don't quite understand the various helpers for
> examining type, or how types are actually implemented in sparse. I see
> that evaluate_ptr_sub does expr->ctype = ssize_t_ctype and return
> ssize_t_ctype, so something else must be going on.

Yeah...  This is a bug in Smatch not Sparse.  The problem is that Sparse
was never really designed to be used how Smatch does it so sometimes the
ctype information isn't set up.  Sparse wants people to use the
evaluated tree, but then information like sizeof() is gone and replaced
with literals.  Anway, sometimes it's there and sometimes not.  When
it's not there I cobbled it together in smatch_type.c.

I'm sort of surprised that Sparse does this:

struct symbol *ssize_t_ctype = &int_ctype;

I really thought it should have been &long_ctype instead...  Anyway,
this patch should fix it.  I've pushed your first four patches.  If this
patch is Ok, then resend me patch 5 and I'll push that as well.

regards,
dan carpenter

diff --git a/smatch_type.c b/smatch_type.c
index bb9a816a..e41a1837 100644
--- a/smatch_type.c
+++ b/smatch_type.c
@@ -71,13 +71,16 @@ static struct symbol *get_binop_type(struct expression *expr)
 			return &int_ctype;
 		return left;
 	}
-	if (left->type == SYM_PTR || left->type == SYM_ARRAY)
-		return left;
-
 	right = get_type(expr->right);
 	if (!right)
 		return NULL;
 
+	if (expr->op == '-' && left->type == SYM_PTR &&
+	    types_equiv(left, right))
+		return ssize_t_ctype;
+
+	if (left->type == SYM_PTR || left->type == SYM_ARRAY)
+		return left;
 	if (right->type == SYM_PTR || right->type == SYM_ARRAY)
 		return right;
 

 	if (!one && !two)

Re: [PATCH 5/5] check_kernel_printf.c: warn about "%lx", (long)ptr

[Rasmus Villemoes]

On 26 October 2017 at 13:51, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> I'm sort of surprised that Sparse does this:
>
> struct symbol *ssize_t_ctype = &int_ctype;
>
> I really thought it should have been &long_ctype instead...

It seems it used to be that, but it was changed 10 years ago
(5061de9). But there's various -mfoo options to tweak it.

>  Anyway,
> this patch should fix it.  I've pushed your first four patches.  If this
> patch is Ok, then resend me patch 5 and I'll push that as well.

Looks ok, I'll see if I can try it out. But now that I look in
smatch_type.c, using this get_binop_type() for EXPR_COMPARE is
definitely wrong, since the type of any ==, <=, >=, <, >, !=
expression is just plain int.

Rasmus