[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 unconvenient or to allow to configure its limit.


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


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

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

-- 
2.13.0

[PATCH 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

[PATCH 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: -Wmemcpy-max-count=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..efbd78d01 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 \fBmemset()\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 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    | 16 ++++++++++++++++
 lib.h    |  1 +
 sparse.1 | 10 ++++++++++
 sparse.c |  3 +--
 4 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/lib.c b/lib.c
index 90fd2b494..1378cc243 100644
--- a/lib.c
+++ b/lib.c
@@ -256,6 +256,7 @@ int dbg_dead = 0;
 
 int fmem_report = 0;
 int fdump_linearize;
+unsigned long fmemcpy_max_count = 100000;
 
 int preprocess_only;
 
@@ -670,6 +671,19 @@ static char **handle_switch_O(char *arg, char **next)
 	return next;
 }
 
+static char **handle_switch_fmemcpy_max_count(char *arg, char **next)
+{
+	unsigned long val;
+	char *end;
+
+	val = strtoul(arg, &end, 0);
+	if (*end != '\0' || end == arg)
+		die("error: missing argument to \"-fmemcpy-max-count=\"");
+
+	fmemcpy_max_count = val;
+	return next;
+}
+
 static char **handle_switch_ftabstop(char *arg, char **next)
 {
 	char *end;
@@ -713,6 +727,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..b7cb451e0 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 fmemcpy_max_count;
 
 extern int arch_m64;
 
diff --git a/sparse.1 b/sparse.1
index efbd78d01..932ac82ef 100644
--- a/sparse.1
+++ b/sparse.1
@@ -216,6 +216,9 @@ Warn about call of \fBmemset()\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,13 @@ Report some statistics about memory allocation used by the tool.
 .
 .SH OTHER OPTIONS
 .TP
+.B \-fmemcpy-limit=COUNT
+By default, sparse will warn if \fBmemcpy()\fR (or \fBmemset()\fR,
+\fBcopy_from_user()\fR, copy_to_user()\fR) is called with a very large
+(known at compile-time) byte-count. COUNT is the value under which
+no such warning will be given. 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 01/06/17 21:27, 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 unconvenient or to allow to configure its limit.
> 
> 
> The series can also be found on the tree:
> 	git://github.com/lucvoo/sparse.git memcpy-max-count

Heh, I think you know I wrote a similar patch about a decade ago ...

[I tried to find it the other day but couldn't find it - I think
it is on my old laptop, which doesn't get booted up these days!]

ATB,
Ramsay Jones

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

[Ramsay Jones]

On 01/06/17 21:27, Luc Van Oostenryck 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.
> 
> However these functions take an unsigned byte count (size_t)
> and so the value can't really be negative.

My patch wasn't as careful as this. (and I was too lazy to check
that the kernel functions took a size_t parameter).

Assuming all functions take a size_t parameter, looks good.

ATB,
Ramsay Jones

> 
> 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;
>  	}
> 

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

[Ramsay Jones]

On 01/06/17 21:27, Luc Van Oostenryck wrote:
> 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: -Wmemcpy-max-count=COUNT.

As always, the hardest part for me is naming. I can't remember exactly
what I called it, but I wanted something which would kinda sum up
all the functions. So, it was something like -Wmem-move-limit=n, or
something like that, and so the test was disabled by setting it to
zero.

-Wmemcpy-max-count=count is probably a better name! ;-)

> 
> 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..efbd78d01 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 \fBmemset()\fR, \fBmemset()\fR, \fBcopy_from_user()\fR, or
                        ^^^^^^^         ^^^^^^^^
memcpy() and memset()

ATB,
Ramsay Jones

> +\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;
> 

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

[Luc Van Oostenryck]

On Fri, Jun 2, 2017 at 2:11 AM, Ramsay Jones
<ramsay@ramsayjones.plus.com> wrote:
>
>
> On 01/06/17 21:27, 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 unconvenient or to allow to configure its limit.
>>
>>
>> The series can also be found on the tree:
>>       git://github.com/lucvoo/sparse.git memcpy-max-count
>
> Heh, I think you know I wrote a similar patch about a decade ago ...
>
> [I tried to find it the other day but couldn't find it - I think
> it is on my old laptop, which doesn't get booted up these days!]

More or less, yes. I saw it in the email where you and Junio talked
about how this memset() warning was a bit blocking for git to use
sparse, which is the motivation of these 3 patches.

-- Luc

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

[Ramsay Jones]

On 01/06/17 21:27, Luc Van Oostenryck wrote:
> 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    | 16 ++++++++++++++++
>  lib.h    |  1 +
>  sparse.1 | 10 ++++++++++
>  sparse.c |  3 +--
>  4 files changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/lib.c b/lib.c
> index 90fd2b494..1378cc243 100644
> --- a/lib.c
> +++ b/lib.c
> @@ -256,6 +256,7 @@ int dbg_dead = 0;
>  
>  int fmem_report = 0;
>  int fdump_linearize;
> +unsigned long fmemcpy_max_count = 100000;
>  
>  int preprocess_only;
>  
> @@ -670,6 +671,19 @@ static char **handle_switch_O(char *arg, char **next)
>  	return next;
>  }
>  
> +static char **handle_switch_fmemcpy_max_count(char *arg, char **next)
> +{
> +	unsigned long val;
> +	char *end;
> +
> +	val = strtoul(arg, &end, 0);
> +	if (*end != '\0' || end == arg)
> +		die("error: missing argument to \"-fmemcpy-max-count=\"");
> +
> +	fmemcpy_max_count = val;
> +	return next;
> +}
> +
>  static char **handle_switch_ftabstop(char *arg, char **next)
>  {
>  	char *end;
> @@ -713,6 +727,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..b7cb451e0 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 fmemcpy_max_count;
>  
>  extern int arch_m64;
>  
> diff --git a/sparse.1 b/sparse.1
> index efbd78d01..932ac82ef 100644
> --- a/sparse.1
> +++ b/sparse.1
> @@ -216,6 +216,9 @@ Warn about call of \fBmemset()\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,13 @@ Report some statistics about memory allocation used by the tool.
>  .
>  .SH OTHER OPTIONS
>  .TP
> +.B \-fmemcpy-limit=COUNT
> +By default, sparse will warn if \fBmemcpy()\fR (or \fBmemset()\fR,
> +\fBcopy_from_user()\fR, copy_to_user()\fR) is called with a very large
> +(known at compile-time) byte-count. COUNT is the value under which
> +no such warning will be given. The default limit is 100000.
> +.
> +.TP

So, in addition to -Wno-memcpy-max-count, you could turn the warning
off with just -fmemcpy-limit=0. cool.

Thanks!

ATB,
Ramsay Jones

>  .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)
> -
> +		if (Wmemcpy_max_count && val > fmemcpy_max_count)
>  			warning(insn->pos, "%s with byte count of %llu",
>  				show_ident(insn->func->sym->ident), val);
>  		return;
> 

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

[Luc Van Oostenryck]

On Fri, Jun 2, 2017 at 2:23 AM, Ramsay Jones
<ramsay@ramsayjones.plus.com> wrote:
>
>
> On 01/06/17 21:27, Luc Van Oostenryck wrote:
>> 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: -Wmemcpy-max-count=COUNT.
>
> As always, the hardest part for me is naming. I can't remember exactly
> what I called it, but I wanted something which would kinda sum up
> all the functions. So, it was something like -Wmem-move-limit=n, or
> something like that, and so the test was disabled by setting it to
> zero.

Yes, the name is far from ideal. I used 'memcpy' for the name because
I think it's the most representative but I would like to have a better name.
I considered for a while something like '-Wmem-operations' or '-Wmem-functions',
now I'm just thinking about '-Wmem-transfert-limit=...', I dunno.

> -Wmemcpy-max-count=count is probably a better name! ;-)

>> diff --git a/sparse.1 b/sparse.1
>> index c924b3a59..efbd78d01 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 \fBmemset()\fR, \fBmemset()\fR, \fBcopy_from_user()\fR, or
>                         ^^^^^^^         ^^^^^^^^
> memcpy() and memset()

Ah yes, good catch. Thanks.


-- Luc

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

[Ramsay Jones]

On 02/06/17 01:30, Ramsay Jones wrote:
> 
> 
> On 01/06/17 21:27, Luc Van Oostenryck wrote:
>> 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    | 16 ++++++++++++++++
>>  lib.h    |  1 +
>>  sparse.1 | 10 ++++++++++
>>  sparse.c |  3 +--
>>  4 files changed, 28 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib.c b/lib.c
>> index 90fd2b494..1378cc243 100644
>> --- a/lib.c
>> +++ b/lib.c
>> @@ -256,6 +256,7 @@ int dbg_dead = 0;
>>  
>>  int fmem_report = 0;
>>  int fdump_linearize;
>> +unsigned long fmemcpy_max_count = 100000;
>>  
>>  int preprocess_only;
>>  
>> @@ -670,6 +671,19 @@ static char **handle_switch_O(char *arg, char **next)
>>  	return next;
>>  }
>>  
>> +static char **handle_switch_fmemcpy_max_count(char *arg, char **next)
>> +{
>> +	unsigned long val;
>> +	char *end;
>> +
>> +	val = strtoul(arg, &end, 0);
>> +	if (*end != '\0' || end == arg)
>> +		die("error: missing argument to \"-fmemcpy-max-count=\"");
>> +
>> +	fmemcpy_max_count = val;
>> +	return next;
>> +}
>> +
>>  static char **handle_switch_ftabstop(char *arg, char **next)
>>  {
>>  	char *end;
>> @@ -713,6 +727,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..b7cb451e0 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 fmemcpy_max_count;
>>  
>>  extern int arch_m64;
>>  
>> diff --git a/sparse.1 b/sparse.1
>> index efbd78d01..932ac82ef 100644
>> --- a/sparse.1
>> +++ b/sparse.1
>> @@ -216,6 +216,9 @@ Warn about call of \fBmemset()\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,13 @@ Report some statistics about memory allocation used by the tool.
>>  .
>>  .SH OTHER OPTIONS
>>  .TP
>> +.B \-fmemcpy-limit=COUNT
>> +By default, sparse will warn if \fBmemcpy()\fR (or \fBmemset()\fR,
>> +\fBcopy_from_user()\fR, copy_to_user()\fR) is called with a very large
>> +(known at compile-time) byte-count. COUNT is the value under which
>> +no such warning will be given. The default limit is 100000.
>> +.
>> +.TP
> 
> So, in addition to -Wno-memcpy-max-count, you could turn the warning
> off with just -fmemcpy-limit=0. cool.

heh, so I obviously didn't read the code! Ahem. :-D

Thanks again.

ATB,
Ramsay Jones

>>  .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)
>> -
>> +		if (Wmemcpy_max_count && val > fmemcpy_max_count)
>>  			warning(insn->pos, "%s with byte count of %llu",
>>  				show_ident(insn->func->sym->ident), val);
>>  		return;
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

[Luc Van Oostenryck]

On Fri, Jun 2, 2017 at 2:30 AM, Ramsay Jones
<ramsay@ramsayjones.plus.com> wrote:

>>  .SH OTHER OPTIONS
>>  .TP
>> +.B \-fmemcpy-limit=COUNT
>> +By default, sparse will warn if \fBmemcpy()\fR (or \fBmemset()\fR,
>> +\fBcopy_from_user()\fR, copy_to_user()\fR) is called with a very large
>> +(known at compile-time) byte-count. COUNT is the value under which
>> +no such warning will be given. The default limit is 100000.
>> +.
>> +.TP
>
> So, in addition to -Wno-memcpy-max-count, you could turn the warning
> off with just -fmemcpy-limit=0. cool.
>
> Thanks!
>
> ATB,
> Ramsay Jones

Well, for now setting the limit to 0 would just warn about any
non-zero memcpy/memset
but it's something that could very easily be added, sure.

-- Luc

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

[Luc Van Oostenryck]

On Fri, Jun 2, 2017 at 2:16 AM, Ramsay Jones
<ramsay@ramsayjones.plus.com> wrote:
>
> Assuming all functions take a size_t parameter, looks good.

Even if they/some don't, it will be OK because it's how sparse
interpret the constant
that matter.

-- Luc

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

[Ramsay Jones]

On 02/06/17 01:38, Luc Van Oostenryck wrote:
> On Fri, Jun 2, 2017 at 2:30 AM, Ramsay Jones
> <ramsay@ramsayjones.plus.com> wrote:
> 
>>>  .SH OTHER OPTIONS
>>>  .TP
>>> +.B \-fmemcpy-limit=COUNT
>>> +By default, sparse will warn if \fBmemcpy()\fR (or \fBmemset()\fR,
>>> +\fBcopy_from_user()\fR, copy_to_user()\fR) is called with a very large
>>> +(known at compile-time) byte-count. COUNT is the value under which
>>> +no such warning will be given. The default limit is 100000.
>>> +.
>>> +.TP
>>
>> So, in addition to -Wno-memcpy-max-count, you could turn the warning
>> off with just -fmemcpy-limit=0. cool.
>>
>> Thanks!
>>
>> ATB,
>> Ramsay Jones
> 
> Well, for now setting the limit to 0 would just warn about any
> non-zero memcpy/memset
> but it's something that could very easily be added, sure.

Yes, as I noted in another email, I didn't read the code correctly!
(and in my patch I had a single -Wmem-limit=n argument which _did_
disable the check when n = 0).

Naming issues aside (and I'm bad at naming, so don't listen to me
on that), I like your -W[no-]memcpy-max-count and -fmemcpy-limit=n
split.

[You may want to remove the '=COUNT' from the commit message of
the 2/3 patch].

Thanks again.

ATB,
Ramsay Jones

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

[Luc Van Oostenryck]

On Fri, Jun 2, 2017 at 3:42 AM, Ramsay Jones
<ramsay@ramsayjones.plus.com> wrote:

>> Well, for now setting the limit to 0 would just warn about any
>> non-zero memcpy/memset
>> but it's something that could very easily be added, sure.
>
> Yes, as I noted in another email, I didn't read the code correctly!
> (and in my patch I had a single -Wmem-limit=n argument which _did_
> disable the check when n = 0).
>
> Naming issues aside (and I'm bad at naming, so don't listen to me
> on that), I like your -W[no-]memcpy-max-count and -fmemcpy-limit=n
> split.
>
> [You may want to remove the '=COUNT' from the commit message of
> the 2/3 patch].

Oh yes, indeed.
Thanks.

-- Luc

[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

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