[PATCH 0/3] -Wmemcpy-max-count & friends

[PATCH 0/3] -Wmemcpy-max-count & friends

[Luc Van Oostenryck]

sparse will warn if memcpy() (and some others memcpy-like
functions) is called with a very large static byte count.
But this warning cannot be disabled and the limit is arbitrary
fixed at 100000.

The goal of this series is to allow to disable this warning if
found too bothersome or to allow to configure its limit.


Changes since v1:
- take in account Ramsay's remarks and suggestion:
  - fix some name mixups in the man page & commit message
  - use a limit of 0 as being equivalent to an infinite
    limit, effectively disabling the warning.
- somewhat rewrote the man page for -fmemcpy-max-count
- extend the limit's range

The series can also be found on the tree:
	git://github.com/lucvoo/sparse.git memcpy-max-count-v2

Luc Van Oostenryck (3):
  memcpy()'s byte count is unsigned
  add support for -Wmemcpy-max-count
  add support for -fmemcpy-max-count

 lib.c    | 20 ++++++++++++++++++++
 lib.h    |  2 ++
 sparse.1 | 17 +++++++++++++++++
 sparse.c |  6 +++---
 4 files changed, 42 insertions(+), 3 deletions(-)

-- 
2.13.0

[PATCH v2 1/3] memcpy()'s byte count is unsigned

[Luc Van Oostenryck]

The checker part of sparse does some checking on memcpy(),
memset(), copy_{from,to}_user() byte count and warn if the
value is known to be too large. The comparison is done with
signed numbers and it also warns if the value is negative.

However these functions take an unsigned byte count (size_t)
and so the value can't really be negative.

Additionaly, the number of bits used by sparse internally may not
be the same as the one used for the target's size_t. So sparse's
check against negative value may not be the same as checking if
the target's value would be so-large-than-the-upper-bit-is-set.

Change this by removing the test for negative values and simply
do an unsigned compare.

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

diff --git a/sparse.c b/sparse.c
index 02ab97743..1cb90e20d 100644
--- a/sparse.c
+++ b/sparse.c
@@ -152,9 +152,9 @@ static void check_byte_count(struct instruction *insn, pseudo_t count)
 	if (!count)
 		return;
 	if (count->type == PSEUDO_VAL) {
-		long long val = count->value;
-		if (val <= 0 || val > 100000)
-			warning(insn->pos, "%s with byte count of %lld",
+		unsigned long long val = count->value;
+		if (val > 100000ULL)
+			warning(insn->pos, "%s with byte count of %llu",
 				show_ident(insn->func->sym->ident), val);
 		return;
 	}
-- 
2.13.0

Re: [PATCH v2 1/3] memcpy()'s byte count is unsigned

[Christopher Li]

On Sat, Jun 3, 2017 at 12:47 AM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> The checker part of sparse does some checking on memcpy(),
> memset(), copy_{from,to}_user() byte count and warn if the
> value is known to be too large. The comparison is done with
> signed numbers and it also warns if the value is negative.


I see you have the V1 in git tree in the 0/3 email.

Do you have the V2 in the git tree some where as well?
I can't seem to find the V2 version of the 0/3 email.

Chris

Re: [PATCH v2 1/3] memcpy()'s byte count is unsigned

[Luc Van Oostenryck]

On Mon, Jun 5, 2017 at 10:52 PM, Christopher Li <sparse@chrisli.org> wrote:
> On Sat, Jun 3, 2017 at 12:47 AM, Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
>> The checker part of sparse does some checking on memcpy(),
>> memset(), copy_{from,to}_user() byte count and warn if the
>> value is known to be too large. The comparison is done with
>> signed numbers and it also warns if the value is negative.
>
>
> I see you have the V1 in git tree in the 0/3 email.
>
> Do you have the V2 in the git tree some where as well?
> I can't seem to find the V2 version of the 0/3 email.

Ah, sorry, I've cut-&-pasted the cover letter between V1 & V2
and thus removed the 'v2' from the title.

The mail corresponding to V2's cover letter is:
    http://marc.info/?l=linux-sparse&m=149647605600827&w=4
but I'll resent it as a reply to this mail.

-- Luc

Re: [PATCH v2 1/3] memcpy()'s byte count is unsigned

[Christopher Li]

On Mon, Jun 5, 2017 at 3:16 PM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
>
> Ah, sorry, I've cut-&-pasted the cover letter between V1 & V2
> and thus removed the 'v2' from the title.

Ah, I see. I actually read that email before but I forget about it.
I will take a look at the V2 git branch.

Chris

[PATCH v2 0/3] -Wmemcpy-max-count & friends

[Luc Van Oostenryck]

sparse will warn if memcpy() (and some others memcpy-like
functions) is called with a very large static byte count.
But this warning cannot be disabled and the limit is arbitrary
fixed at 100000.

The goal of this series is to allow to disable this warning if
found too bothersome or to allow to configure its limit.


Changes since v1:
- take in account Ramsay's remarks and suggestion:
  - fix some name mixups in the man page & commit message
  - use a limit of 0 as being equivalent to an infinite
    limit, effectively disabling the warning.
- somewhat rewrote the man page for -fmemcpy-max-count
- extend the limit's range

The series can also be found on the tree:
	git://github.com/lucvoo/sparse.git memcpy-max-count-v2

Luc Van Oostenryck (3):
  memcpy()'s byte count is unsigned
  add support for -Wmemcpy-max-count
  add support for -fmemcpy-max-count

 lib.c    | 20 ++++++++++++++++++++
 lib.h    |  2 ++
 sparse.1 | 17 +++++++++++++++++
 sparse.c |  6 +++---
 4 files changed, 42 insertions(+), 3 deletions(-)

[PATCH v2 2/3] add support for -Wmemcpy-max-count

[Luc Van Oostenryck]

sparse will warn if memcpy() (or memset(), copy_from_user(),
copy_to_user()) is called with a very large static byte-count.

But this warning is given unconditionaly while there are projects
where this warning may not be not desired.

Change this by making this warning conditional on a new warning
flag: -W[no-]memcpy-max-count

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 lib.c    | 2 ++
 lib.h    | 1 +
 sparse.1 | 8 ++++++++
 sparse.c | 3 ++-
 4 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/lib.c b/lib.c
index e1e6a1cbf..90fd2b494 100644
--- a/lib.c
+++ b/lib.c
@@ -229,6 +229,7 @@ int Wdo_while = 0;
 int Winit_cstring = 0;
 int Wenum_mismatch = 1;
 int Wsparse_error = 0;
+int Wmemcpy_max_count = 1;
 int Wnon_pointer_null = 1;
 int Wold_initializer = 1;
 int Wone_bit_signed_bitfield = 1;
@@ -506,6 +507,7 @@ static const struct warning {
 	{ "enum-mismatch", &Wenum_mismatch },
 	{ "sparse-error", &Wsparse_error },
 	{ "init-cstring", &Winit_cstring },
+	{ "memcpy-max-count", &Wmemcpy_max_count },
 	{ "non-pointer-null", &Wnon_pointer_null },
 	{ "old-initializer", &Wold_initializer },
 	{ "one-bit-signed-bitfield", &Wone_bit_signed_bitfield },
diff --git a/lib.h b/lib.h
index 2c8529f93..8090fe247 100644
--- a/lib.h
+++ b/lib.h
@@ -116,6 +116,7 @@ extern int Wdo_while;
 extern int Wenum_mismatch;
 extern int Wsparse_error;
 extern int Winit_cstring;
+extern int Wmemcpy_max_count;
 extern int Wnon_pointer_null;
 extern int Wold_initializer;
 extern int Wone_bit_signed_bitfield;
diff --git a/sparse.1 b/sparse.1
index c924b3a59..df3c7f442 100644
--- a/sparse.1
+++ b/sparse.1
@@ -210,6 +210,14 @@ trouble.
 Sparse does not issue these warnings by default.
 .
 .TP
+.B \-Wmemcpy\-max\-count
+Warn about call of \fBmemcpy()\fR, \fBmemset()\fR, \fBcopy_from_user()\fR, or
+\fBcopy_to_user()\fR with a large compile-time byte count.
+
+Sparse issues these warnings by default. To turn them off, use
+\fB\-Wno\-memcpy\-max\-count\fR.
+.
+.TP
 .B \-Wnon\-pointer\-null
 Warn about the use of 0 as a NULL pointer.
 
diff --git a/sparse.c b/sparse.c
index 1cb90e20d..aa5979f1a 100644
--- a/sparse.c
+++ b/sparse.c
@@ -153,7 +153,8 @@ static void check_byte_count(struct instruction *insn, pseudo_t count)
 		return;
 	if (count->type == PSEUDO_VAL) {
 		unsigned long long val = count->value;
-		if (val > 100000ULL)
+		if (Wmemcpy_max_count && val > 100000ULL)
+
 			warning(insn->pos, "%s with byte count of %llu",
 				show_ident(insn->func->sym->ident), val);
 		return;
-- 
2.13.0

[PATCH v2 3/3] add support for -fmemcpy-max-count

[Luc Van Oostenryck]

By default, sparse will warn if memcpy() (or memset(),
copy_from_user(), copy_to_user()) is called with a very large
static byte-count.

But the limit is currently fixed at 100000, which may be fine
for some uses but not for others. For example, this value is
too low for sparse to be used on the git tree where, for example,
some array used to sort the index is cleared with memset().

Change this by making the limit configurable via a new flag:
-fmemcpy-max-count.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 lib.c    | 18 ++++++++++++++++++
 lib.h    |  1 +
 sparse.1 |  9 +++++++++
 sparse.c |  3 +--
 4 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/lib.c b/lib.c
index 90fd2b494..69efbecc9 100644
--- a/lib.c
+++ b/lib.c
@@ -256,6 +256,7 @@ int dbg_dead = 0;
 
 int fmem_report = 0;
 int fdump_linearize;
+unsigned long long fmemcpy_max_count = 100000;
 
 int preprocess_only;
 
@@ -670,6 +671,21 @@ static char **handle_switch_O(char *arg, char **next)
 	return next;
 }
 
+static char **handle_switch_fmemcpy_max_count(char *arg, char **next)
+{
+	unsigned long long val;
+	char *end;
+
+	val = strtoull(arg, &end, 0);
+	if (*end != '\0' || end == arg)
+		die("error: missing argument to \"-fmemcpy-max-count=\"");
+
+	if (val == 0)
+		val = ~0ULL;
+	fmemcpy_max_count = val;
+	return next;
+}
+
 static char **handle_switch_ftabstop(char *arg, char **next)
 {
 	char *end;
@@ -713,6 +729,8 @@ static char **handle_switch_f(char *arg, char **next)
 		return handle_switch_ftabstop(arg+8, next);
 	if (!strncmp(arg, "dump-", 5))
 		return handle_switch_fdump(arg+5, next);
+	if (!strncmp(arg, "memcpy-max-count=", 17))
+		return handle_switch_fmemcpy_max_count(arg+17, next);
 
 	/* handle switches w/ arguments above, boolean and only boolean below */
 	if (handle_simple_switch(arg, "mem-report", &fmem_report))
diff --git a/lib.h b/lib.h
index 8090fe247..6dc4ed244 100644
--- a/lib.h
+++ b/lib.h
@@ -143,6 +143,7 @@ extern int dbg_dead;
 
 extern int fmem_report;
 extern int fdump_linearize;
+extern unsigned long long fmemcpy_max_count;
 
 extern int arch_m64;
 
diff --git a/sparse.1 b/sparse.1
index df3c7f442..b79c58767 100644
--- a/sparse.1
+++ b/sparse.1
@@ -216,6 +216,9 @@ Warn about call of \fBmemcpy()\fR, \fBmemset()\fR, \fBcopy_from_user()\fR, or
 
 Sparse issues these warnings by default. To turn them off, use
 \fB\-Wno\-memcpy\-max\-count\fR.
+
+The limit can be changed with \fB\-fmemcpy\-max\-count=COUNT\fR,
+the default being \fB100000\fR.
 .
 .TP
 .B \-Wnon\-pointer\-null
@@ -364,6 +367,12 @@ Report some statistics about memory allocation used by the tool.
 .
 .SH OTHER OPTIONS
 .TP
+.B \-fmemcpy-max-count=COUNT
+Set the limit for the warnings given by \fB-Wmemcpy-max-count\fR.
+A COUNT of 0, useless in itself, will effectively disable the warning.
+The default limit is 100000.
+.
+.TP
 .B \-ftabstop=WIDTH
 Set the distance between tab stops.  This helps sparse report correct
 column numbers in warnings or errors.  If the value is less than 1 or
diff --git a/sparse.c b/sparse.c
index aa5979f1a..bceacd94e 100644
--- a/sparse.c
+++ b/sparse.c
@@ -153,8 +153,7 @@ static void check_byte_count(struct instruction *insn, pseudo_t count)
 		return;
 	if (count->type == PSEUDO_VAL) {
 		unsigned long long val = count->value;
-		if (Wmemcpy_max_count && val > 100000ULL)

Re: [PATCH 0/3] -Wmemcpy-max-count & friends

[Ramsay Jones]

On 03/06/17 08:47, Luc Van Oostenryck wrote:
> sparse will warn if memcpy() (and some others memcpy-like
> functions) is called with a very large static byte count.
> But this warning cannot be disabled and the limit is arbitrary
> fixed at 100000.
> 
> The goal of this series is to allow to disable this warning if
> found too bothersome or to allow to configure its limit.
> 
> 
> Changes since v1:
> - take in account Ramsay's remarks and suggestion:
>   - fix some name mixups in the man page & commit message
>   - use a limit of 0 as being equivalent to an infinite
>     limit, effectively disabling the warning.
> - somewhat rewrote the man page for -fmemcpy-max-count
> - extend the limit's range
> 
> The series can also be found on the tree:
> 	git://github.com/lucvoo/sparse.git memcpy-max-count-v2

Looks good to me. Thanks!

ATB,
Ramsay Jones

Re: [PATCH 0/3] -Wmemcpy-max-count & friends

[Christopher Li]

On Sat, Jun 3, 2017 at 12:47 AM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> sparse will warn if memcpy() (and some others memcpy-like
> functions) is called with a very large static byte count.
> But this warning cannot be disabled and the limit is arbitrary
> fixed at 100000.
>
> The goal of this series is to allow to disable this warning if
> found too bothersome or to allow to configure its limit.
>
>
> Changes since v1:
> - take in account Ramsay's remarks and suggestion:
>   - fix some name mixups in the man page & commit message
>   - use a limit of 0 as being equivalent to an infinite
>     limit, effectively disabling the warning.
> - somewhat rewrote the man page for -fmemcpy-max-count
> - extend the limit's range
>
> The series can also be found on the tree:
>         git://github.com/lucvoo/sparse.git memcpy-max-count-v2

This V2 version of the series looks perfectly fine to me.

Signed-off-By: Chris Li <sparse@chrisli.org>

Chris