[PATCH] quiet memset() warning with sizeof(VLA)

[PATCH] quiet memset() warning with sizeof(VLA)

[Luc Van Oostenryck]

Currently, sparse doesn't handle yet VLA's sizeof(). The size
of a VLA is considered as zero and the result of sizeof() on
a VLA is treated as an error with a value of -1.

That's a problem with the check done by the sparse tool, which
warn when operations like memset() are done with a static count
which is above some limit. Of course, all this is done with
unsigned numbers and the -1 from sizeof(VLA) is then considered
as off-limit.

Sure, size of VLAs should be supported but it's longer term.
One short term solution would be to do the check with signed
numbers but that would eat the upper bit of the limit which
may well be the bound that we would like to check.

So, check instead for sizes of -1, which must come from some
previous errors that must have already been reported, and do
not issue the memset() warning in this case.

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

diff --git a/sparse.c b/sparse.c
index bceacd94e..5f53a9b9b 100644
--- a/sparse.c
+++ b/sparse.c
@@ -153,6 +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 == -1ULL)
+			return;
 		if (Wmemcpy_max_count && val > fmemcpy_max_count)
 			warning(insn->pos, "%s with byte count of %llu",
 				show_ident(insn->func->sym->ident), val);
-- 
2.16.0

Re: [PATCH] quiet memset() warning with sizeof(VLA)

[Josh Triplett]

On Sat, Feb 17, 2018 at 05:58:39PM +0100, Luc Van Oostenryck wrote:
> Currently, sparse doesn't handle yet VLA's sizeof(). The size
> of a VLA is considered as zero and the result of sizeof() on
> a VLA is treated as an error with a value of -1.
> 
> That's a problem with the check done by the sparse tool, which
> warn when operations like memset() are done with a static count
> which is above some limit. Of course, all this is done with
> unsigned numbers and the -1 from sizeof(VLA) is then considered
> as off-limit.
> 
> Sure, size of VLAs should be supported but it's longer term.
> One short term solution would be to do the check with signed
> numbers but that would eat the upper bit of the limit which
> may well be the bound that we would like to check.
> 
> So, check instead for sizes of -1, which must come from some
> previous errors that must have already been reported, and do
> not issue the memset() warning in this case.

I'm concerned about generically ignoring this warning for *all* -1
sizes, because -1 seems like a very common value to slip through for
other reasons.  Losing those very real warnings just to avoid getting a
second warning on a VLA doesn't seem worth it.

(Also, at the very *least* this would need a comment explaining *why* it
ignores -1.)

Could you somehow propagate a taintedness on that value that causes the
memset to ignore it? Or just change the dummy error value to 0?

>  sparse.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/sparse.c b/sparse.c
> index bceacd94e..5f53a9b9b 100644
> --- a/sparse.c
> +++ b/sparse.c
> @@ -153,6 +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 == -1ULL)
> +			return;
>  		if (Wmemcpy_max_count && val > fmemcpy_max_count)
>  			warning(insn->pos, "%s with byte count of %llu",
>  				show_ident(insn->func->sym->ident), val);
> -- 
> 2.16.0
> 
> --
> 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] quiet memset() warning with sizeof(VLA)

[Luc Van Oostenryck]

On Sat, Feb 17, 2018 at 11:18:26AM -0800, Josh Triplett wrote:
> On Sat, Feb 17, 2018 at 05:58:39PM +0100, Luc Van Oostenryck wrote:
> > Currently, sparse doesn't handle yet VLA's sizeof(). The size
> > of a VLA is considered as zero and the result of sizeof() on
> > a VLA is treated as an error with a value of -1.
> > 
> > That's a problem with the check done by the sparse tool, which
> > warn when operations like memset() are done with a static count
> > which is above some limit. Of course, all this is done with
> > unsigned numbers and the -1 from sizeof(VLA) is then considered
> > as off-limit.
> > 
> > Sure, size of VLAs should be supported but it's longer term.
> > One short term solution would be to do the check with signed
> > numbers but that would eat the upper bit of the limit which
> > may well be the bound that we would like to check.
> > 
> > So, check instead for sizes of -1, which must come from some
> > previous errors that must have already been reported, and do
> > not issue the memset() warning in this case.
> 
> I'm concerned about generically ignoring this warning for *all* -1
> sizes, because -1 seems like a very common value to slip through for
> other reasons.  Losing those very real warnings just to avoid getting a
> second warning on a VLA doesn't seem worth it.

Yes, I think you're right.
 
> (Also, at the very *least* this would need a comment explaining *why* it
> ignores -1.)
> 
> Could you somehow propagate a taintedness on that value that causes the
> memset to ignore it? Or just change the dummy error value to 0?

Yes, that could be a solution (but changing it to zero seems to me as
bad). I'll look for something but I think I'll just better spend a bit
time on having real support for VLA's size.

Thanks for the review
-- Luc