[PATCH] Handle SForced in storage_modifiers

[PATCH] Handle SForced in storage_modifiers

[Jeff Layton]

We have been seeing errors like this for a while now in the sparse
Fedora package, when doing kernel builds:

    ./include/linux/err.h:53:25: warning: dereference of noderef expression
    ./include/linux/err.h:35:16: warning: dereference of noderef expression

This spews all over the build because this comes from IS_ERR(), which
is called everywhere. Even odder, it turns out that if we build the
package with -fpic turned off, then it works fine.

With some brute-force debugging, I think I've finally found the cause.
This array is missing the SForced element. When this is added then the
problem goes away.

As to why this goes away when -fpic is removed, I can only assume that
we get lucky with the memory layout and have a zeroed out region just
beyond the end of the array.

Fixes: 3829c4d8b097776e6b3472290a9fae08a705ab7a
Cc: Al Viro <viro@ftp.linux.org.uk>
Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 parse.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/parse.c b/parse.c
index 205e12644a6c..b290ff2636f2 100644
--- a/parse.c
+++ b/parse.c
@@ -1286,7 +1286,8 @@ static unsigned long storage_modifiers(struct decl_state *ctx)
 		[SAuto] = MOD_AUTO,
 		[SExtern] = MOD_EXTERN,
 		[SStatic] = MOD_STATIC,
-		[SRegister] = MOD_REGISTER
+		[SRegister] = MOD_REGISTER,
+		[SForced] = 0
 	};
 	return mod[ctx->storage_class] | (ctx->is_inline ? MOD_INLINE : 0)
 		| (ctx->is_tls ? MOD_TLS : 0);
-- 
2.7.4

Re: [PATCH] Handle SForced in storage_modifiers

[Luc Van Oostenryck]

On Mon, Nov 14, 2016 at 02:12:16PM -0500, Jeff Layton wrote:
> We have been seeing errors like this for a while now in the sparse
> Fedora package, when doing kernel builds:
> 
>     ./include/linux/err.h:53:25: warning: dereference of noderef expression
>     ./include/linux/err.h:35:16: warning: dereference of noderef expression
> 
> This spews all over the build because this comes from IS_ERR(), which
> is called everywhere. Even odder, it turns out that if we build the
> package with -fpic turned off, then it works fine.
> 
> With some brute-force debugging, I think I've finally found the cause.
> This array is missing the SForced element. When this is added then the
> problem goes away.
> 
> As to why this goes away when -fpic is removed, I can only assume that
> we get lucky with the memory layout and have a zeroed out region just
> beyond the end of the array.
> 
> Fixes: 3829c4d8b097776e6b3472290a9fae08a705ab7a
> Cc: Al Viro <viro@ftp.linux.org.uk>
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  parse.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/parse.c b/parse.c
> index 205e12644a6c..b290ff2636f2 100644
> --- a/parse.c
> +++ b/parse.c
> @@ -1286,7 +1286,8 @@ static unsigned long storage_modifiers(struct decl_state *ctx)
>  		[SAuto] = MOD_AUTO,
>  		[SExtern] = MOD_EXTERN,
>  		[SStatic] = MOD_STATIC,
> -		[SRegister] = MOD_REGISTER
> +		[SRegister] = MOD_REGISTER,
> +		[SForced] = 0
>  	};
>  	return mod[ctx->storage_class] | (ctx->is_inline ? MOD_INLINE : 0)
>  		| (ctx->is_tls ? MOD_TLS : 0);
> -- 

Humm,

The array is statically initialized and never modified,
your patch shouldn't change anything, and this regardless of
the memory layout or compiler options.


Luc Van Oostenryck

Re: [PATCH] Handle SForced in storage_modifiers

[Linus Torvalds]

On Mon, Nov 14, 2016 at 12:04 PM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
>
> The array is statically initialized and never modified,
> your patch shouldn't change anything, and this regardless of
> the memory layout or compiler options.

The problem is the _size_ of the array. Without that initializer for
SForced case, it is one entry too small, and you get a random access
past the end of the array.

The patch is definitely correct.

              Linus

Re: [PATCH] Handle SForced in storage_modifiers

[Luc Van Oostenryck]

On Mon, Nov 14, 2016 at 12:17:47PM -0800, Linus Torvalds wrote:
> On Mon, Nov 14, 2016 at 12:04 PM, Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
> >
> > The array is statically initialized and never modified,
> > your patch shouldn't change anything, and this regardless of
> > the memory layout or compiler options.
> 
> The problem is the _size_ of the array. Without that initializer for
> SForced case, it is one entry too small, and you get a random access
> past the end of the array.
> 
> The patch is definitely correct.
> 
>               Linus
> --

Ah yes, indeed.

Thanks

Re: [PATCH] Handle SForced in storage_modifiers

[Christopher Li]

On Tue, Nov 15, 2016 at 4:17 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> The problem is the _size_ of the array. Without that initializer for
> SForced case, it is one entry too small, and you get a random access
> past the end of the array.
>
> The patch is definitely correct.
>

Yes, the patch is definitely correct. It is a good catch.

I purpose a slightly different way to fix it. I think it is better just give the
array of a size instead of using the designated initializer to determine the
array size. Sequential initializer to determine the array size is fine.

Jeff, how about some thing like this:

Chris

diff --git a/parse.c b/parse.c
index 66f9353..a01ba00 100644
--- a/parse.c
+++ b/parse.c
@@ -109,7 +109,7 @@ enum {
 };

 enum {
-       SNone = 0, STypedef, SAuto, SRegister, SExtern, SStatic, SForced
+       SNone = 0, STypedef, SAuto, SRegister, SExtern, SStatic, SForced, SMax
 };

 static struct symbol_op typedef_op = {
@@ -1279,7 +1279,7 @@ static const char *storage_class[] =

 static unsigned long storage_modifiers(struct decl_state *ctx)
 {
-       static unsigned long mod[] =
+       static unsigned long mod[SMax] =
        {
                [SAuto] = MOD_AUTO,
                [SExtern] = MOD_EXTERN,

Re: [PATCH] Handle SForced in storage_modifiers

[Jeff Layton]

On Tue, 2016-11-15 at 09:00 +0800, Christopher Li wrote:
> On Tue, Nov 15, 2016 at 4:17 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> > The problem is the _size_ of the array. Without that initializer for
> > SForced case, it is one entry too small, and you get a random access
> > past the end of the array.
> > 
> > The patch is definitely correct.
> > 
> 
> Yes, the patch is definitely correct. It is a good catch.
> 
> I purpose a slightly different way to fix it. I think it is better just give the
> array of a size instead of using the designated initializer to determine the
> array size. Sequential initializer to determine the array size is fine.
> 
> Jeff, how about some thing like this:
> 
> Chris
> 
> diff --git a/parse.c b/parse.c
> index 66f9353..a01ba00 100644
> --- a/parse.c
> +++ b/parse.c
> @@ -109,7 +109,7 @@ enum {
>  };
> 
>  enum {
> -       SNone = 0, STypedef, SAuto, SRegister, SExtern, SStatic, SForced
> +       SNone = 0, STypedef, SAuto, SRegister, SExtern, SStatic, SForced, SMax
>  };
> 
>  static struct symbol_op typedef_op = {
> @@ -1279,7 +1279,7 @@ static const char *storage_class[] =
> 
>  static unsigned long storage_modifiers(struct decl_state *ctx)
>  {
> -       static unsigned long mod[] =
> +       static unsigned long mod[SMax] =
>         {
>                 [SAuto] = MOD_AUTO,
>                 [SExtern] = MOD_EXTERN,

That looks fine to me. If you want to merge that one, then:

Reviewed-by: Jeff Layton <jlayton@redhat.com>

Re: [PATCH] Handle SForced in storage_modifiers

[Christopher Li]

On Tue, Nov 15, 2016 at 10:07 AM, Jeff Layton <jlayton@redhat.com> wrote:
>
> That looks fine to me. If you want to merge that one, then:
>
> Reviewed-by: Jeff Layton <jlayton@redhat.com>

The patches is applied on "chrisl" repo and pushed out.

Actually I want to merge the patch having you as the author.

Chris

Re: [PATCH] Handle SForced in storage_modifiers

[Jeff Layton]

On Thu, 2016-11-17 at 06:28 +0800, Christopher Li wrote:
> On Tue, Nov 15, 2016 at 10:07 AM, Jeff Layton <jlayton@redhat.com> wrote:
> > 
> > 
> > That looks fine to me. If you want to merge that one, then:
> > 
> > Reviewed-by: Jeff Layton <jlayton@redhat.com>
> 
> The patches is applied on "chrisl" repo and pushed out.
> 
> Actually I want to merge the patch having you as the author.
> 
> Chris

Looks good to me. Thanks!
-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH] Handle SForced in storage_modifiers

[Luc Van Oostenryck]

On Thu, Nov 17, 2016 at 06:28:15AM +0800, Christopher Li wrote:
> On Tue, Nov 15, 2016 at 10:07 AM, Jeff Layton <jlayton@redhat.com> wrote:
> >
> > That looks fine to me. If you want to merge that one, then:
> >
> > Reviewed-by: Jeff Layton <jlayton@redhat.com>
> 
> The patches is applied on "chrisl" repo and pushed out.
> 
> Actually I want to merge the patch having you as the author.
> 
> Chris
> --


I just noticed now, while doing some merging, that what have been commited
on the master tree is not the patch discussed here.
What have been commited is the same change but with 'CMax' (integer class)
while it was supposed to be 'SMax' (for the array of specifier).
The version on sparse-next is correct though.

Here is a patch fixing this:

From 04ffa624446bda7fbbc2c98fa012cb6e4bac1c53 Mon Sep 17 00:00:00 2001
From: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
Date: Thu, 5 Jan 2017 07:31:46 +0100
Subject: [PATCH] fix mixup in "Handle SForced in storage_modifiers"

The patch used the wrong enum for the maximum size of the array.

Fixes: 1db3b627 ("Handle SForced in storage_modifiers")
Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 parse.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/parse.c b/parse.c
index b52c6abe..385e40ce 100644
--- a/parse.c
+++ b/parse.c
@@ -105,11 +105,11 @@ enum {
 };
 
 enum {
-	CInt = 0, CSInt, CUInt, CReal, CChar, CSChar, CUChar, CMax,
+	CInt = 0, CSInt, CUInt, CReal, CChar, CSChar, CUChar,
 };
 
 enum {
-	SNone = 0, STypedef, SAuto, SRegister, SExtern, SStatic, SForced
+	SNone = 0, STypedef, SAuto, SRegister, SExtern, SStatic, SForced, SMax,
 };
 
 static struct symbol_op typedef_op = {
@@ -1281,7 +1281,7 @@ static const char *storage_class[] =
 
 static unsigned long storage_modifiers(struct decl_state *ctx)
 {
-	static unsigned long mod[CMax] =
+	static unsigned long mod[SMax] =
 	{
 		[SAuto] = MOD_AUTO,
 		[SExtern] = MOD_EXTERN,
-- 
2.11.0

Re: [PATCH] Handle SForced in storage_modifiers

[Josh Triplett]

On Tue, Nov 15, 2016 at 09:00:30AM +0800, Christopher Li wrote:
> On Tue, Nov 15, 2016 at 4:17 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> > The problem is the _size_ of the array. Without that initializer for
> > SForced case, it is one entry too small, and you get a random access
> > past the end of the array.
> >
> > The patch is definitely correct.
> >
> 
> Yes, the patch is definitely correct. It is a good catch.
> 
> I purpose a slightly different way to fix it. I think it is better just give the
> array of a size instead of using the designated initializer to determine the
> array size. Sequential initializer to determine the array size is fine.
> 
> Jeff, how about some thing like this:
> 
> Chris
> 
> diff --git a/parse.c b/parse.c
> index 66f9353..a01ba00 100644
> --- a/parse.c
> +++ b/parse.c
> @@ -109,7 +109,7 @@ enum {
>  };
> 
>  enum {
> -       SNone = 0, STypedef, SAuto, SRegister, SExtern, SStatic, SForced
> +       SNone = 0, STypedef, SAuto, SRegister, SExtern, SStatic, SForced, SMax
>  };
> 
>  static struct symbol_op typedef_op = {
> @@ -1279,7 +1279,7 @@ static const char *storage_class[] =
> 
>  static unsigned long storage_modifiers(struct decl_state *ctx)
>  {
> -       static unsigned long mod[] =
> +       static unsigned long mod[SMax] =
>         {
>                 [SAuto] = MOD_AUTO,
>                 [SExtern] = MOD_EXTERN,

I like this much better as well.

(Having "maximum" entries in enums does sadly break warning options like
-Wswitch-enum, but it makes cases like this much better.)