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]>
---
.../lustre/lustre/obdclass/lprocfs_status.c | 24 +++++++++++---------
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
index 61e04af..5227324 100644
--- a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
+++ b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
@@ -1897,17 +1897,19 @@ int lprocfs_write_frac_u64_helper(const char *buffer, unsigned long count,
}
units = 1;
- switch (*end) {
- case 'p': case 'P':
- units <<= 10;
- case 't': case 'T':
- units <<= 10;
- case 'g': case 'G':
- units <<= 10;
- case 'm': case 'M':
- units <<= 10;
- case 'k': case 'K':
- units <<= 10;
+ if (end) {
+ switch (*end) {
+ case 'p': case 'P':
+ units <<= 10;
+ case 't': case 'T':
+ units <<= 10;
+ case 'g': case 'G':
+ units <<= 10;
+ case 'm': case 'M':
+ units <<= 10;
+ case 'k': case 'K':
+ units <<= 10;
+ }
}
/* Specified units override the multiplier */
if (units)
--
1.7.10.4
On Sun, 2014-12-14 at 23:52 +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.
Perhaps the tool could use a little work.
It's not possible for end to be NULL no?
unsigned long long simple_strtoull(const char *cp, char **endp, unsigned int base)
{
unsigned long long result;
unsigned int rv;
cp = _parse_integer_fixup_radix(cp, &base);
rv = _parse_integer(cp, base, &result);
/* FIXME */
cp += (rv & ~KSTRTOX_OVERFLOW);
if (endp)
*endp = (char *)cp;
return result;
}
EXPORT_SYMBOL(simple_strtoull);
> diff --git a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
[]
Above this:
whole = simple_strtoull(pbuf, &end, 10);
> +++ b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
> @@ -1897,17 +1897,19 @@ int lprocfs_write_frac_u64_helper(const char *buffer, unsigned long count,
> }
>
> units = 1;
> - switch (*end) {
> - case 'p': case 'P':
> - units <<= 10;
> - case 't': case 'T':
> - units <<= 10;
> - case 'g': case 'G':
> - units <<= 10;
> - case 'm': case 'M':
> - units <<= 10;
> - case 'k': case 'K':
> - units <<= 10;
> + if (end) {
> + switch (*end) {
> + case 'p': case 'P':
> + units <<= 10;
> + case 't': case 'T':
> + units <<= 10;
> + case 'g': case 'G':
> + units <<= 10;
> + case 'm': case 'M':
> + units <<= 10;
> + case 'k': case 'K':
> + units <<= 10;
> + }
The only thing I might do is
switch (tolower(*end)) {
and remove the second case entry for each line
Hi Joe
No, it does not look like end can be NULL then.
Then remove the end != NULL instead?
...
if (end != NULL && *end == '.') {
However, I am hesitant to the tolower() I think double case is faster...?
Kind regards
Rickard Strandqvist
2014-12-15 2:51 GMT+01:00 Joe Perches <[email protected]>:
> On Sun, 2014-12-14 at 23:52 +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.
>
> Perhaps the tool could use a little work.
> It's not possible for end to be NULL no?
>
> unsigned long long simple_strtoull(const char *cp, char **endp, unsigned int base)
> {
> unsigned long long result;
> unsigned int rv;
>
> cp = _parse_integer_fixup_radix(cp, &base);
> rv = _parse_integer(cp, base, &result);
> /* FIXME */
> cp += (rv & ~KSTRTOX_OVERFLOW);
>
> if (endp)
> *endp = (char *)cp;
>
> return result;
> }
> EXPORT_SYMBOL(simple_strtoull);
>
>
>> diff --git a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
> []
>
> Above this:
>
> whole = simple_strtoull(pbuf, &end, 10);
>
>> +++ b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
>> @@ -1897,17 +1897,19 @@ int lprocfs_write_frac_u64_helper(const char *buffer, unsigned long count,
>> }
>>
>> units = 1;
>> - switch (*end) {
>> - case 'p': case 'P':
>> - units <<= 10;
>> - case 't': case 'T':
>> - units <<= 10;
>> - case 'g': case 'G':
>> - units <<= 10;
>> - case 'm': case 'M':
>> - units <<= 10;
>> - case 'k': case 'K':
>> - units <<= 10;
>> + if (end) {
>> + switch (*end) {
>> + case 'p': case 'P':
>> + units <<= 10;
>> + case 't': case 'T':
>> + units <<= 10;
>> + case 'g': case 'G':
>> + units <<= 10;
>> + case 'm': case 'M':
>> + units <<= 10;
>> + case 'k': case 'K':
>> + units <<= 10;
>> + }
>
> The only thing I might do is
>
> switch (tolower(*end)) {
>
> and remove the second case entry for each line
>
On Mon, 2014-12-15 at 23:23 +0100, Rickard Strandqvist wrote:
> Hi Joe
Hello Rickard
> No, it does not look like end can be NULL then.
> Then remove the end != NULL instead?
> ...
> if (end != NULL && *end == '.') {
Up to you.
> However, I am hesitant to the tolower() I think double case is faster...?
I doubt code execution speed is paramount here.
Maybe see if the object code size is smaller one
way or the other.
Strongly agreed that execution speed is not critical here. It's the update of a proc variable, not a tight loop or critical section.
Normally I'd leave it alone, but since you're writing a patch anyway, I'd do 'tolower' myself. I dislike the stacked case statements on a single line like that. (It's the only time I've seen them written that way. Perhaps it's common and I've just missed it.)
Regards,
- Patrick
________________________________________
From: HPDD-discuss [[email protected]] on behalf of Joe Perches [[email protected]]
Sent: Monday, December 15, 2014 5:53 PM
To: Rickard Strandqvist
Cc: [email protected]; Fabian Frederick; Julia Lawall; James Simmons; Greg Kroah-Hartman; [email protected]; Greg Donald; [email protected]; Andriy Skulysh
Subject: Re: [HPDD-discuss] [PATCH] staging: lustre: lustre: obdclass: lprocfs_status.c: Fix for possible null pointer dereference
On Mon, 2014-12-15 at 23:23 +0100, Rickard Strandqvist wrote:
> Hi Joe
Hello Rickard
> No, it does not look like end can be NULL then.
> Then remove the end != NULL instead?
> ...
> if (end != NULL && *end == '.') {
Up to you.
> However, I am hesitant to the tolower() I think double case is faster...?
I doubt code execution speed is paramount here.
Maybe see if the object code size is smaller one
way or the other.
_______________________________________________
HPDD-discuss mailing list
[email protected]
https://lists.01.org/mailman/listinfo/hpdd-discuss
On Sun, Dec 14, 2014 at 4:52 PM, Rickard Strandqvist
<[email protected]> wrote:
> units = 1;
...
> /* Specified units override the multiplier */
> if (units)
That doesn't make much sense. The conditional logic will always be
executed. Maybe this is what's intended?
/* Specified units override the multiplier */
- if (units)
+ if (units > 1)
mult = mult < 0 ? -units : units;
Chris
On Mon, Dec 15, 2014 at 11:23:25PM +0100, Rickard Strandqvist wrote:
> Hi Joe
>
> No, it does not look like end can be NULL then.
> Then remove the end != NULL instead?
> ...
> if (end != NULL && *end == '.') {
>
Yes. Please do.
regards,
dan carpenter