[PATCH 1/1] mmap: fix uninitialized entities warnings.

[PATCH 1/1] mmap: fix uninitialized entities warnings.

[Ricardo Martins]

This is my first patch for the Linux kernel. It fixes the following
warnings given by gcc 4.3.0 about uninitialzed entities in mmap.c:

mm/mmap.c: In function ‘insert_vm_struct’:
mm/mmap.c:2079: warning: ‘rb_parent’ may be used uninitialized in this function
mm/mmap.c:2079: warning: ‘rb_link’ may be used uninitialized in this function
mm/mmap.c:2078: warning: ‘prev’ may be used uninitialized in this function
mm/mmap.c: In function ‘copy_vma’:
mm/mmap.c:2118: warning: ‘rb_parent’ may be used uninitialized in this function
mm/mmap.c:2118: warning: ‘rb_link’ may be used uninitialized in this function
mm/mmap.c:2117: warning: ‘prev’ may be used uninitialized in this function
mm/mmap.c: In function ‘do_brk’:
mm/mmap.c:1945: warning: ‘rb_parent’ may be used uninitialized in this function
mm/mmap.c:1945: warning: ‘rb_link’ may be used uninitialized in this function
mm/mmap.c:1943: warning: ‘prev’ may be used uninitialized in this function
mm/mmap.c: In function ‘mmap_region’:
mm/mmap.c:1086: warning: ‘rb_parent’ may be used uninitialized in this function
mm/mmap.c:1086: warning: ‘rb_link’ may be used uninitialized in this function
mm/mmap.c:1083: warning: ‘prev’ may be used uninitialized in this function

I'd appreciate some feedback.

Signed-off-by: Ricardo Martins <ricardo@scarybox.net>
---
 mm/mmap.c |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index fac6633..64b7df1 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -59,7 +59,7 @@ static void unmap_region(struct mm_struct *mm,
  * MAP_SHARED	r: (no) no	r: (yes) yes	r: (no) yes	r: (no) yes
  *		w: (no) no	w: (no) no	w: (yes) yes	w: (no) no
  *		x: (no) no	x: (no) yes	x: (no) yes	x: (yes) yes
- *		
+ *
  * MAP_PRIVATE	r: (no) no	r: (yes) yes	r: (no) yes	r: (no) yes
  *		w: (no) no	w: (no) no	w: (copy) copy	w: (no) no
  *		x: (no) no	x: (no) yes	x: (no) yes	x: (yes) yes
@@ -1080,10 +1080,10 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
 			  int accountable)
 {
 	struct mm_struct *mm = current->mm;
-	struct vm_area_struct *vma, *prev;
+	struct vm_area_struct *vma, *prev = NULL;
 	int correct_wcount = 0;
 	int error;
-	struct rb_node **rb_link, *rb_parent;
+	struct rb_node **rb_link = NULL, *rb_parent = NULL;
 	unsigned long charged = 0;
 	struct inode *inode =  file ? file->f_path.dentry->d_inode : NULL;
 
@@ -1299,7 +1299,7 @@ full_search:
 		addr = vma->vm_end;
 	}
 }
-#endif	
+#endif
 
 void arch_unmap_area(struct mm_struct *mm, unsigned long addr)
 {
@@ -1940,9 +1940,9 @@ static inline void verify_mm_writelocked(struct mm_struct *mm)
 unsigned long do_brk(unsigned long addr, unsigned long len)
 {
 	struct mm_struct * mm = current->mm;
-	struct vm_area_struct * vma, * prev;
+	struct vm_area_struct * vma, * prev = NULL;
 	unsigned long flags;
-	struct rb_node ** rb_link, * rb_parent;
+	struct rb_node ** rb_link = NULL, * rb_parent = NULL;
 	pgoff_t pgoff = addr >> PAGE_SHIFT;
 	int error;
 
@@ -2075,8 +2075,8 @@ void exit_mmap(struct mm_struct *mm)
  */
 int insert_vm_struct(struct mm_struct * mm, struct vm_area_struct * vma)
 {
-	struct vm_area_struct * __vma, * prev;
-	struct rb_node ** rb_link, * rb_parent;
+	struct vm_area_struct * __vma, * prev = NULL;
+	struct rb_node ** rb_link = NULL, * rb_parent = NULL;
 
 	/*
 	 * The vm_pgoff of a purely anonymous vma should be irrelevant
@@ -2114,8 +2114,8 @@ struct vm_area_struct *copy_vma(struct vm_area_struct **vmap,
 	struct vm_area_struct *vma = *vmap;
 	unsigned long vma_start = vma->vm_start;
 	struct mm_struct *mm = vma->vm_mm;
-	struct vm_area_struct *new_vma, *prev;
-	struct rb_node **rb_link, *rb_parent;
+	struct vm_area_struct *new_vma, *prev = NULL;
+	struct rb_node **rb_link = NULL, *rb_parent = NULL;
 	struct mempolicy *pol;
 
 	/*
-- 
1.5.5.1

Re: [PATCH 1/1] mmap: fix uninitialized entities warnings.

[Matthew Wilcox]

On Thu, May 01, 2008 at 08:06:52PM +0100, Ricardo Martins wrote:
> This is my first patch for the Linux kernel. It fixes the following
> warnings given by gcc 4.3.0 about uninitialzed entities in mmap.c:

First, it's generally a bad idea to fix these warnings.  They sometimes
obscure real bugs.

> I'd appreciate some feedback.

>   *		x: (no) no	x: (no) yes	x: (no) yes	x: (yes) yes
> - *		
> + *

Don't mix whitespace changes with other changes.

> -	struct vm_area_struct *vma, *prev;
> +	struct vm_area_struct *vma, *prev = NULL;

gcc bug -- it's failing to notice that find_vma_prepare() will always
initialise pprev.

> -	struct rb_node **rb_link, *rb_parent;
> +	struct rb_node **rb_link = NULL, *rb_parent = NULL;

Likewise.

The other changes in your patch are either whitespace or fixes for the
same gcc problem.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH 1/1] mmap: fix uninitialized entities warnings.

[Adrian Bunk]

On Thu, May 01, 2008 at 01:16:27PM -0600, Matthew Wilcox wrote:
> On Thu, May 01, 2008 at 08:06:52PM +0100, Ricardo Martins wrote:
> > This is my first patch for the Linux kernel. It fixes the following
> > warnings given by gcc 4.3.0 about uninitialzed entities in mmap.c:
> 
> First, it's generally a bad idea to fix these warnings.  They sometimes
> obscure real bugs.
>...

ACK

> > -	struct vm_area_struct *vma, *prev;
> > +	struct vm_area_struct *vma, *prev = NULL;
> 
> gcc bug -- it's failing to notice that find_vma_prepare() will always
> initialise pprev.
>...

You miss the "return" inside the while() loop in find_vma_prepare().

I do not know whether the code is correct, but the answer is not that 
easy.

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH 1/1] mmap: fix uninitialized entities warnings.

[Ricardo Martins]

[-- Attachment #1: Type: text/plain, Size: 1181 bytes --]

On Thu, 01 May 2008 13:16, Matthew Wilcox wrote:
> On Thu, May 01, 2008 at 08:06:52PM +0100, Ricardo Martins wrote:
> > This is my first patch for the Linux kernel. It fixes the following
> > warnings given by gcc 4.3.0 about uninitialzed entities in mmap.c:
>
> First, it's generally a bad idea to fix these warnings.  They sometimes
> obscure real bugs.

I see.

> > I'd appreciate some feedback.
>
> >   *		x: (no) no	x: (no) yes	x: (no) yes	x: (yes) yes
> > - *
> > + *
>
> Don't mix whitespace changes with other changes.

Ok, it won't happen again.

> > -	struct vm_area_struct *vma, *prev;
> > +	struct vm_area_struct *vma, *prev = NULL;
>
> gcc bug -- it's failing to notice that find_vma_prepare() will always
> initialise pprev.
>
> > -	struct rb_node **rb_link, *rb_parent;
> > +	struct rb_node **rb_link = NULL, *rb_parent = NULL;
>
> Likewise.

Hmm, that crossed my mind and I thought it was weird that gcc was
complaining about it.

> The other changes in your patch are either whitespace or fixes for the
> same gcc problem.

Thanks for your feedback; it was very helpful, since I'm a C newbie.

Regards,
--
 Ricardo Martins  *  scarybox.net  *  GPG key: 0x1308F1B4

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 1/1] mmap: fix uninitialized entities warnings.

[Matthew Wilcox]

On Thu, May 01, 2008 at 10:43:25PM +0300, Adrian Bunk wrote:
> > > -	struct vm_area_struct *vma, *prev;
> > > +	struct vm_area_struct *vma, *prev = NULL;
> > 
> > gcc bug -- it's failing to notice that find_vma_prepare() will always
> > initialise pprev.
> >...
> 
> You miss the "return" inside the while() loop in find_vma_prepare().
> 
> I do not know whether the code is correct, but the answer is not that 
> easy.

You're right, it isn't that easy.

However, the code is not buggy.  Just compare:

                        vma = vma_tmp;
                        if (vma_tmp->vm_start <= addr)
                                return vma;

with

munmap_back:
        vma = find_vma_prepare(mm, addr, &prev, &rb_link, &rb_parent);
        if (vma && vma->vm_start < addr + len) {
                if (do_munmap(mm, addr, len))
                        return -ENOMEM;
                goto munmap_back;
        }

Now, we know that addr + len does not wrap, and that len > 0, so we know
that this warning is incorrect.  But it's not reasonable to expect gcc to
be able to deduce this.

See also the CERT warning threads recently on the GCC lists ;-)

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH 1/1] mmap: fix uninitialized entities warnings.

[Adrian Bunk]

On Thu, May 01, 2008 at 02:25:33PM -0600, Matthew Wilcox wrote:
> On Thu, May 01, 2008 at 10:43:25PM +0300, Adrian Bunk wrote:
> > > > -	struct vm_area_struct *vma, *prev;
> > > > +	struct vm_area_struct *vma, *prev = NULL;
> > > 
> > > gcc bug -- it's failing to notice that find_vma_prepare() will always
> > > initialise pprev.
> > >...
> > 
> > You miss the "return" inside the while() loop in find_vma_prepare().
> > 
> > I do not know whether the code is correct, but the answer is not that 
> > easy.
> 
> You're right, it isn't that easy.
> 
> However, the code is not buggy.  Just compare:
> 
>                         vma = vma_tmp;
>                         if (vma_tmp->vm_start <= addr)
>                                 return vma;
> 
> with
> 
> munmap_back:
>         vma = find_vma_prepare(mm, addr, &prev, &rb_link, &rb_parent);
>         if (vma && vma->vm_start < addr + len) {
>                 if (do_munmap(mm, addr, len))
>                         return -ENOMEM;
>                 goto munmap_back;
>         }
> 
> Now, we know that addr + len does not wrap, and that len > 0, so we know
> that this warning is incorrect.  But it's not reasonable to expect gcc to
> be able to deduce this.
>...

Can you submit a patch with your explanation that uses 
uninitialized_var() to silence these warnings?

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH 1/1] mmap: fix uninitialized entities warnings.

[Matthew Wilcox]

On Thu, May 01, 2008 at 11:40:37PM +0300, Adrian Bunk wrote:
> On Thu, May 01, 2008 at 02:25:33PM -0600, Matthew Wilcox wrote:
> > However, the code is not buggy.  Just compare:
> > 
> >                         vma = vma_tmp;
> >                         if (vma_tmp->vm_start <= addr)
> >                                 return vma;
> > 
> > with
> > 
> > munmap_back:
> >         vma = find_vma_prepare(mm, addr, &prev, &rb_link, &rb_parent);
> >         if (vma && vma->vm_start < addr + len) {
> >                 if (do_munmap(mm, addr, len))
> >                         return -ENOMEM;
> >                 goto munmap_back;
> >         }
> > 
> > Now, we know that addr + len does not wrap, and that len > 0, so we know
> > that this warning is incorrect.  But it's not reasonable to expect gcc to
> > be able to deduce this.
> >...
> 
> Can you submit a patch with your explanation that uses 
> uninitialized_var() to silence these warnings?

I could, but I don't know if I want to.  Suppose somebody changes the
code later to actually make prev used before it's initialised?  Then we
would have GCC not warning about a problem.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH 1/1] mmap: fix uninitialized entities warnings.

[Adrian Bunk]

On Thu, May 01, 2008 at 03:05:19PM -0600, Matthew Wilcox wrote:
> On Thu, May 01, 2008 at 11:40:37PM +0300, Adrian Bunk wrote:
> > On Thu, May 01, 2008 at 02:25:33PM -0600, Matthew Wilcox wrote:
> > > However, the code is not buggy.  Just compare:
> > > 
> > >                         vma = vma_tmp;
> > >                         if (vma_tmp->vm_start <= addr)
> > >                                 return vma;
> > > 
> > > with
> > > 
> > > munmap_back:
> > >         vma = find_vma_prepare(mm, addr, &prev, &rb_link, &rb_parent);
> > >         if (vma && vma->vm_start < addr + len) {
> > >                 if (do_munmap(mm, addr, len))
> > >                         return -ENOMEM;
> > >                 goto munmap_back;
> > >         }
> > > 
> > > Now, we know that addr + len does not wrap, and that len > 0, so we know
> > > that this warning is incorrect.  But it's not reasonable to expect gcc to
> > > be able to deduce this.
> > >...
> > 
> > Can you submit a patch with your explanation that uses 
> > uninitialized_var() to silence these warnings?
> 
> I could, but I don't know if I want to.  Suppose somebody changes the
> code later to actually make prev used before it's initialised?  Then we
> would have GCC not warning about a problem.

That's why we silence the warnings with uninitialized_var() - simply 
change the #define and you'll see all silenced warnings.

The problem is that these warnings for which we know that the code is OK 
today make it harder to spot where new warnings get added that might 
indicate real bugs.

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed