2015-01-02 18:55 GMT+01:00 Ashley Pittman <apittman@ddn.com>:

Rickard,

> On 21 Dec 2014, at 22:43, Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se> wrote:
>
> The NULL check was done to late, and there it was a risk
> of a possible null pointer dereference.
>
> This was partially found by using a static code analysis program called cppcheck.
>
> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
> ---
> drivers/staging/lustre/lustre/include/lustre_update.h |    4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/lustre/lustre/include/lustre_update.h b/drivers/staging/lustre/lustre/include/lustre_update.h
> index 84defce..00e1361 100644
> --- a/drivers/staging/lustre/lustre/include/lustre_update.h
> +++ b/drivers/staging/lustre/lustre/include/lustre_update.h
> @@ -165,12 +165,14 @@ static inline int update_get_reply_buf(struct update_reply *reply, void **buf,
>       int  result;
>
>       ptr = update_get_buf_internal(reply, index, &size);
> +
> +     LASSERT((ptr != NULL && size >= sizeof(int)));
> +
>       result = *(int *)ptr;
>
>       if (result < 0)
>               return result;
>
> -     LASSERT((ptr != NULL && size >= sizeof(int)));

This looks odd to me, LASSERT is essentially BUG_ON() so is used for checking logic bugs.  Moving LASSERT calls doesn’t seem the correct way of resolving a logic problem and if you’re doing static analysis it might be more productive to do it this with LASSERT disabled.

>       *buf = ptr + sizeof(int);
>       return size - sizeof(int);
> }
> --
> 1.7.10.4
>
> _______________________________________________
> HPDD-discuss mailing list
> HPDD-discuss@lists.01.org
> https://lists.01.org/mailman/listinfo/hpdd-discuss


Hi

But even if this is only for debugging purposes, it is still not appropriate to perform null test before using it?


cppcheck running with all different define active.



Kind regards
Rickard Strandqvist