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 <[email protected]>
---
drivers/staging/lustre/lustre/osc/lproc_osc.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/lustre/lustre/osc/lproc_osc.c b/drivers/staging/lustre/lustre/osc/lproc_osc.c
index 9f719bc..9ba6293 100644
--- a/drivers/staging/lustre/lustre/osc/lproc_osc.c
+++ b/drivers/staging/lustre/lustre/osc/lproc_osc.c
@@ -237,13 +237,15 @@ static ssize_t osc_cur_grant_bytes_seq_write(struct file *file, const char *buff
size_t count, loff_t *off)
{
struct obd_device *obd = ((struct seq_file *)file->private_data)->private;
- struct client_obd *cli = &obd->u.cli;
+ struct client_obd *cli;
int rc;
__u64 val;
if (obd == NULL)
return 0;
+ cli = &obd->u.cli;
+
rc = lprocfs_write_u64_helper(buffer, count, &val);
if (rc)
return rc;
--
1.7.10.4
On Sun, Dec 14, 2014 at 11:37:18PM +0100, Rickard Strandqvist wrote:
> 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 <[email protected]>
> ---
> drivers/staging/lustre/lustre/osc/lproc_osc.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/lustre/lustre/osc/lproc_osc.c b/drivers/staging/lustre/lustre/osc/lproc_osc.c
> index 9f719bc..9ba6293 100644
> --- a/drivers/staging/lustre/lustre/osc/lproc_osc.c
> +++ b/drivers/staging/lustre/lustre/osc/lproc_osc.c
> @@ -237,13 +237,15 @@ static ssize_t osc_cur_grant_bytes_seq_write(struct file *file, const char *buff
> size_t count, loff_t *off)
> {
> struct obd_device *obd = ((struct seq_file *)file->private_data)->private;
> - struct client_obd *cli = &obd->u.cli;
> + struct client_obd *cli;
This isn't really a dereference. You're just getting the address of
obd->u.cli. So if obd is NULL then it won't crash.
regards,
dan carpenter
Hi Dan
Quite right! Had to try it.
Do nothing then?
But you must agree that it is still ugly and confusing code.
Kind regards
Rickard Strandqvist
2014-12-15 11:25 GMT+01:00 Dan Carpenter <[email protected]>:
> On Sun, Dec 14, 2014 at 11:37:18PM +0100, Rickard Strandqvist wrote:
>> 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 <[email protected]>
>> ---
>> drivers/staging/lustre/lustre/osc/lproc_osc.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/lustre/lustre/osc/lproc_osc.c b/drivers/staging/lustre/lustre/osc/lproc_osc.c
>> index 9f719bc..9ba6293 100644
>> --- a/drivers/staging/lustre/lustre/osc/lproc_osc.c
>> +++ b/drivers/staging/lustre/lustre/osc/lproc_osc.c
>> @@ -237,13 +237,15 @@ static ssize_t osc_cur_grant_bytes_seq_write(struct file *file, const char *buff
>> size_t count, loff_t *off)
>> {
>> struct obd_device *obd = ((struct seq_file *)file->private_data)->private;
>> - struct client_obd *cli = &obd->u.cli;
>> + struct client_obd *cli;
>
> This isn't really a dereference. You're just getting the address of
> obd->u.cli. So if obd is NULL then it won't crash.
>
> regards,
> dan carpenter
>
On Tue, Dec 16, 2014 at 12:07:19AM +0100, Rickard Strandqvist wrote:
> Hi Dan
>
> Quite right! Had to try it.
>
> Do nothing then?
> But you must agree that it is still ugly and confusing code.
>
Yes. I agree that it's confusing. I also suspect that "obd" is never
NULL but I haven't actually looked and these things are sometimes
complicated to verify.
I fine with merging the patch as a cleanup.
Smatch has code to not warn about these but it's not 100% correct so
it still warns sometimes when it shouldn't.
regards,
dan carpenter