[PATCH] arch: metag: mm: hugetlbpage.c: Cleaning up inconsistent NULL checks

[PATCH] arch: metag: mm: hugetlbpage.c: Cleaning up inconsistent NULL checks

[Rickard Strandqvist]

Cleaning up inconsistent NULL checks.
There is otherwise a risk of a possible null pointer dereference.

Was largely found by using a static code analysis program called cppcheck.

Signed-off-by: Rickard Strandqvist <rickard_strandqvist-IW2WV5XWFqGZkjO+N0TKoMugMpMbD5Xr@public.gmane.org>
---
 arch/metag/mm/hugetlbpage.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/metag/mm/hugetlbpage.c b/arch/metag/mm/hugetlbpage.c
index 0424315..3f8d5cd 100644
--- a/arch/metag/mm/hugetlbpage.c
+++ b/arch/metag/mm/hugetlbpage.c
@@ -188,7 +188,8 @@ new_search:
 			}
 		}
 		after_huge = 0;
-		addr = ALIGN_HUGEPT(vma->vm_end);
+		if (vma)
+			addr = ALIGN_HUGEPT(vma->vm_end);
 	}
 }
 #endif
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-metag" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] arch: metag: mm: hugetlbpage.c: Cleaning up inconsistent NULL checks

[Rickard Strandqvist]

Cleaning up inconsistent NULL checks.
There is otherwise a risk of a possible null pointer dereference.

Was largely found by using a static code analysis program called cppcheck.

Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
---
 arch/metag/mm/hugetlbpage.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/metag/mm/hugetlbpage.c b/arch/metag/mm/hugetlbpage.c
index 0424315..3f8d5cd 100644
--- a/arch/metag/mm/hugetlbpage.c
+++ b/arch/metag/mm/hugetlbpage.c
@@ -188,7 +188,8 @@ new_search:
 			}
 		}
 		after_huge = 0;
-		addr = ALIGN_HUGEPT(vma->vm_end);
+		if (vma)
+			addr = ALIGN_HUGEPT(vma->vm_end);
 	}
 }
 #endif
-- 
1.7.10.4

Re: [PATCH] arch: metag: mm: hugetlbpage.c: Cleaning up inconsistent NULL checks

[James Hogan]

Hi Rickard,

On 22/05/14 23:01, Rickard Strandqvist wrote:
> Cleaning up inconsistent NULL checks.
> There is otherwise a risk of a possible null pointer dereference.
> 
> Was largely found by using a static code analysis program called cppcheck.
> 
> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
> ---
>  arch/metag/mm/hugetlbpage.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/metag/mm/hugetlbpage.c b/arch/metag/mm/hugetlbpage.c
> index 0424315..3f8d5cd 100644
> --- a/arch/metag/mm/hugetlbpage.c
> +++ b/arch/metag/mm/hugetlbpage.c
> @@ -188,7 +188,8 @@ new_search:
>  			}
>  		}
>  		after_huge = 0;
> -		addr = ALIGN_HUGEPT(vma->vm_end);
> +		if (vma)
> +			addr = ALIGN_HUGEPT(vma->vm_end);
>  	}
>  }
>  #endif
> 

I don't think this is a correct fix.

If !vma && !after_huge the first if block in the loop will match and the
function will return 0.
If !vma && after_huge the 3rd if block in the loop will match and the
function will return addr.

So removing the vma condition on the final if block in the loop would
probably make sense instead. Does that satisfy cppcheck?

Cheers
James

Re: [PATCH] arch: metag: mm: hugetlbpage.c: Cleaning up inconsistent NULL checks

[James Hogan]

Hi Rickard,

On 22/05/14 23:01, Rickard Strandqvist wrote:
> Cleaning up inconsistent NULL checks.
> There is otherwise a risk of a possible null pointer dereference.
> 
> Was largely found by using a static code analysis program called cppcheck.
> 
> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
> ---
>  arch/metag/mm/hugetlbpage.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/metag/mm/hugetlbpage.c b/arch/metag/mm/hugetlbpage.c
> index 0424315..3f8d5cd 100644
> --- a/arch/metag/mm/hugetlbpage.c
> +++ b/arch/metag/mm/hugetlbpage.c
> @@ -188,7 +188,8 @@ new_search:
>  			}
>  		}
>  		after_huge = 0;
> -		addr = ALIGN_HUGEPT(vma->vm_end);
> +		if (vma)
> +			addr = ALIGN_HUGEPT(vma->vm_end);
>  	}
>  }
>  #endif
> 

I don't think this is a correct fix.

If !vma && !after_huge the first if block in the loop will match and the
function will return 0.
If !vma && after_huge the 3rd if block in the loop will match and the
function will return addr.

So removing the vma condition on the final if block in the loop would
probably make sense instead. Does that satisfy cppcheck?

Cheers
James

Re: [PATCH] arch: metag: mm: hugetlbpage.c: Cleaning up inconsistent NULL checks

[Rickard Strandqvist]

Hi

First, I do not think we'll adapt the code as a static code checking to work!
This must be resolved by other means.

But yes, it is control where that gets cppcheck to react. And it is
then also an unnecessary control. So then I guess it's ok to remove
it.
I resend a patch, with it removed.


Best regards
Rickard Strandqvist


2014-05-28 12:03 GMT+02:00 James Hogan <james.hogan@imgtec.com>:
> Hi Rickard,
>
> On 22/05/14 23:01, Rickard Strandqvist wrote:
>> Cleaning up inconsistent NULL checks.
>> There is otherwise a risk of a possible null pointer dereference.
>>
>> Was largely found by using a static code analysis program called cppcheck.
>>
>> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
>> ---
>>  arch/metag/mm/hugetlbpage.c |    3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/metag/mm/hugetlbpage.c b/arch/metag/mm/hugetlbpage.c
>> index 0424315..3f8d5cd 100644
>> --- a/arch/metag/mm/hugetlbpage.c
>> +++ b/arch/metag/mm/hugetlbpage.c
>> @@ -188,7 +188,8 @@ new_search:
>>                       }
>>               }
>>               after_huge = 0;
>> -             addr = ALIGN_HUGEPT(vma->vm_end);
>> +             if (vma)
>> +                     addr = ALIGN_HUGEPT(vma->vm_end);
>>       }
>>  }
>>  #endif
>>
>
> I don't think this is a correct fix.
>
> If !vma && !after_huge the first if block in the loop will match and the
> function will return 0.
> If !vma && after_huge the 3rd if block in the loop will match and the
> function will return addr.
>
> So removing the vma condition on the final if block in the loop would
> probably make sense instead. Does that satisfy cppcheck?
>
> Cheers
> James