* [U-Boot] [PATCH 1/1] ubifs: avoid possible NULL dereference
@ 2017-11-21 18:45 Heinrich Schuchardt
2017-11-21 20:23 ` Ladislav Michl
2017-11-21 21:16 ` Wolfgang Denk
0 siblings, 2 replies; 10+ messages in thread
From: Heinrich Schuchardt @ 2017-11-21 18:45 UTC (permalink / raw)
To: u-boot
If 'file' cannot be allocated due to an out of memory
situation, do not dereference it.
When debugging this patch also avoids a misleading message
"cannot find next direntry, error %d" in case of an out of
memory situation. It is sufficent to write
"%s: Error, no memory for malloc!\n" in this case.
Reported-by: Alex Sadovsky <nable.maininbox@googlemail.com>
Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
fs/ubifs/ubifs.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/fs/ubifs/ubifs.c b/fs/ubifs/ubifs.c
index 4465523d5f..313dee0579 100644
--- a/fs/ubifs/ubifs.c
+++ b/fs/ubifs/ubifs.c
@@ -403,8 +403,7 @@ static int ubifs_finddir(struct super_block *sb, char *dirname,
dir = kzalloc(sizeof(struct inode), 0);
if (!file || !dentry || !dir) {
printf("%s: Error, no memory for malloc!\n", __func__);
- err = -ENOMEM;
- goto out;
+ goto out_nomem;
}
dir->i_sb = sb;
@@ -424,7 +423,6 @@ static int ubifs_finddir(struct super_block *sb, char *dirname,
err = PTR_ERR(dent);
goto out;
}
-
file->f_pos = key_hash_flash(c, &dent->key);
file->private_data = dent;
@@ -463,6 +461,7 @@ out:
out_free:
kfree(file->private_data);
+out_nomem:
free(file);
free(dentry);
free(dir);
--
2.11.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* [U-Boot] [PATCH 1/1] ubifs: avoid possible NULL dereference
2017-11-21 18:45 [U-Boot] [PATCH 1/1] ubifs: avoid possible NULL dereference Heinrich Schuchardt
@ 2017-11-21 20:23 ` Ladislav Michl
2017-11-21 21:29 ` Heinrich Schuchardt
2017-11-21 21:16 ` Wolfgang Denk
1 sibling, 1 reply; 10+ messages in thread
From: Ladislav Michl @ 2017-11-21 20:23 UTC (permalink / raw)
To: u-boot
On Tue, Nov 21, 2017 at 07:45:03PM +0100, Heinrich Schuchardt wrote:
> If 'file' cannot be allocated due to an out of memory
> situation, do not dereference it.
>
> When debugging this patch also avoids a misleading message
> "cannot find next direntry, error %d" in case of an out of
> memory situation. It is sufficent to write
> "%s: Error, no memory for malloc!\n" in this case.
>
> Reported-by: Alex Sadovsky <nable.maininbox@googlemail.com>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> fs/ubifs/ubifs.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/fs/ubifs/ubifs.c b/fs/ubifs/ubifs.c
> index 4465523d5f..313dee0579 100644
> --- a/fs/ubifs/ubifs.c
> +++ b/fs/ubifs/ubifs.c
> @@ -403,8 +403,7 @@ static int ubifs_finddir(struct super_block *sb, char *dirname,
> dir = kzalloc(sizeof(struct inode), 0);
> if (!file || !dentry || !dir) {
> printf("%s: Error, no memory for malloc!\n", __func__);
> - err = -ENOMEM;
> - goto out;
> + goto out_nomem;
> }
>
> dir->i_sb = sb;
> @@ -424,7 +423,6 @@ static int ubifs_finddir(struct super_block *sb, char *dirname,
> err = PTR_ERR(dent);
> goto out;
> }
> -
> file->f_pos = key_hash_flash(c, &dent->key);
> file->private_data = dent;
>
This hunk should be probably droped.
> @@ -463,6 +461,7 @@ out:
>
> out_free:
> kfree(file->private_data);
Now question is why is file->private_data ever assigned as it seems nothing
works with it.
> +out_nomem:
> free(file);
> free(dentry);
> free(dir);
> --
> 2.11.0
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot
^ permalink raw reply [flat|nested] 10+ messages in thread* [U-Boot] [PATCH 1/1] ubifs: avoid possible NULL dereference
2017-11-21 20:23 ` Ladislav Michl
@ 2017-11-21 21:29 ` Heinrich Schuchardt
2017-11-21 21:55 ` Ladislav Michl
0 siblings, 1 reply; 10+ messages in thread
From: Heinrich Schuchardt @ 2017-11-21 21:29 UTC (permalink / raw)
To: u-boot
On 11/21/2017 09:23 PM, Ladislav Michl wrote:
> On Tue, Nov 21, 2017 at 07:45:03PM +0100, Heinrich Schuchardt wrote:
>> If 'file' cannot be allocated due to an out of memory
>> situation, do not dereference it.
>>
>> When debugging this patch also avoids a misleading message
>> "cannot find next direntry, error %d" in case of an out of
>> memory situation. It is sufficent to write
>> "%s: Error, no memory for malloc!\n" in this case.
>>
>> Reported-by: Alex Sadovsky <nable.maininbox@googlemail.com>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>> fs/ubifs/ubifs.c | 5 ++---
>> 1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/ubifs/ubifs.c b/fs/ubifs/ubifs.c
>> index 4465523d5f..313dee0579 100644
>> --- a/fs/ubifs/ubifs.c
>> +++ b/fs/ubifs/ubifs.c
>> @@ -403,8 +403,7 @@ static int ubifs_finddir(struct super_block *sb, char *dirname,
>> dir = kzalloc(sizeof(struct inode), 0);
>> if (!file || !dentry || !dir) {
>> printf("%s: Error, no memory for malloc!\n", __func__);
>> - err = -ENOMEM;
>> - goto out;
>> + goto out_nomem;
>> }
>>
>> dir->i_sb = sb;
>> @@ -424,7 +423,6 @@ static int ubifs_finddir(struct super_block *sb, char *dirname,
>> err = PTR_ERR(dent);
>> goto out;
>> }
>> -
>> file->f_pos = key_hash_flash(c, &dent->key);
>> file->private_data = dent;
>>
>
> This hunk should be probably droped.
>
>> @@ -463,6 +461,7 @@ out:
>>
>> out_free:
>> kfree(file->private_data);
>
> Now question is why is file->private_data ever assigned as it seems nothing
> works with it.
Good catch.
Do we need file and dentry at all in the function?
Unfortunately the file cannot be compiled on x86 cf.
https://lists.denx.de/pipermail/u-boot/2017-November/312166.html
Best regards
Heinrich
>
>> +out_nomem:
>> free(file);
>> free(dentry);
>> free(dir);
>> --
>> 2.11.0
>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> https://lists.denx.de/listinfo/u-boot
>
^ permalink raw reply [flat|nested] 10+ messages in thread* [U-Boot] [PATCH 1/1] ubifs: avoid possible NULL dereference
2017-11-21 21:29 ` Heinrich Schuchardt
@ 2017-11-21 21:55 ` Ladislav Michl
0 siblings, 0 replies; 10+ messages in thread
From: Ladislav Michl @ 2017-11-21 21:55 UTC (permalink / raw)
To: u-boot
On Tue, Nov 21, 2017 at 10:29:51PM +0100, Heinrich Schuchardt wrote:
>
>
> On 11/21/2017 09:23 PM, Ladislav Michl wrote:
> > On Tue, Nov 21, 2017 at 07:45:03PM +0100, Heinrich Schuchardt wrote:
> > > If 'file' cannot be allocated due to an out of memory
> > > situation, do not dereference it.
> > >
> > > When debugging this patch also avoids a misleading message
> > > "cannot find next direntry, error %d" in case of an out of
> > > memory situation. It is sufficent to write
> > > "%s: Error, no memory for malloc!\n" in this case.
> > >
> > > Reported-by: Alex Sadovsky <nable.maininbox@googlemail.com>
> > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > > ---
> > > fs/ubifs/ubifs.c | 5 ++---
> > > 1 file changed, 2 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/fs/ubifs/ubifs.c b/fs/ubifs/ubifs.c
> > > index 4465523d5f..313dee0579 100644
> > > --- a/fs/ubifs/ubifs.c
> > > +++ b/fs/ubifs/ubifs.c
> > > @@ -403,8 +403,7 @@ static int ubifs_finddir(struct super_block *sb, char *dirname,
> > > dir = kzalloc(sizeof(struct inode), 0);
> > > if (!file || !dentry || !dir) {
> > > printf("%s: Error, no memory for malloc!\n", __func__);
> > > - err = -ENOMEM;
> > > - goto out;
> > > + goto out_nomem;
> > > }
> > > dir->i_sb = sb;
> > > @@ -424,7 +423,6 @@ static int ubifs_finddir(struct super_block *sb, char *dirname,
> > > err = PTR_ERR(dent);
> > > goto out;
> > > }
> > > -
> > > file->f_pos = key_hash_flash(c, &dent->key);
> > > file->private_data = dent;
> > >
> > This hunk should be probably droped.
> >
> > > @@ -463,6 +461,7 @@ out:
> > > out_free:
> > > kfree(file->private_data);
> >
> > Now question is why is file->private_data ever assigned as it seems nothing
> > works with it.
>
> Good catch.
>
> Do we need file and dentry at all in the function?
Does not seem so. Also what f_pos is good for is unclear.
> Unfortunately the file cannot be compiled on x86 cf.
> https://lists.denx.de/pipermail/u-boot/2017-November/312166.html
I could give it a try on ARM board. Care to cook an untested patch?
> Best regards
>
> Heinrich
>
>
> >
> > > +out_nomem:
> > > free(file);
> > > free(dentry);
> > > free(dir);
> > > --
> > > 2.11.0
> > >
> > > _______________________________________________
> > > U-Boot mailing list
> > > U-Boot at lists.denx.de
> > > https://lists.denx.de/listinfo/u-boot
> >
^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH 1/1] ubifs: avoid possible NULL dereference
2017-11-21 18:45 [U-Boot] [PATCH 1/1] ubifs: avoid possible NULL dereference Heinrich Schuchardt
2017-11-21 20:23 ` Ladislav Michl
@ 2017-11-21 21:16 ` Wolfgang Denk
2017-11-21 21:22 ` Ladislav Michl
2017-11-21 21:28 ` Heinrich Schuchardt
1 sibling, 2 replies; 10+ messages in thread
From: Wolfgang Denk @ 2017-11-21 21:16 UTC (permalink / raw)
To: u-boot
Dear Heinrich,
In message <20171121184503.3193-1-xypron.glpk@gmx.de> you wrote:
> If 'file' cannot be allocated due to an out of memory
> situation, do not dereference it.
>
> When debugging this patch also avoids a misleading message
> "cannot find next direntry, error %d" in case of an out of
> memory situation. It is sufficent to write
> "%s: Error, no memory for malloc!\n" in this case.
>
> Reported-by: Alex Sadovsky <nable.maininbox@googlemail.com>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> fs/ubifs/ubifs.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/fs/ubifs/ubifs.c b/fs/ubifs/ubifs.c
> index 4465523d5f..313dee0579 100644
> --- a/fs/ubifs/ubifs.c
> +++ b/fs/ubifs/ubifs.c
> @@ -403,8 +403,7 @@ static int ubifs_finddir(struct super_block *sb, char *dirname,
> dir = kzalloc(sizeof(struct inode), 0);
> if (!file || !dentry || !dir) {
> printf("%s: Error, no memory for malloc!\n", __func__);
> - err = -ENOMEM;
> - goto out;
> + goto out_nomem;
> }
>
> dir->i_sb = sb;
> @@ -424,7 +423,6 @@ static int ubifs_finddir(struct super_block *sb, char *dirname,
> err = PTR_ERR(dent);
> goto out;
> }
> -
> file->f_pos = key_hash_flash(c, &dent->key);
> file->private_data = dent;
>
> @@ -463,6 +461,7 @@ out:
>
> out_free:
> kfree(file->private_data);
> +out_nomem:
> free(file);
> free(dentry);
> free(dir);
Should you not keep the "err = -ENOMEM;" setting? Otherwise there
is no indivcation that an error happened.
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
There are no data that cannot be plotted on a straight line if the
axis are chosen correctly.
^ permalink raw reply [flat|nested] 10+ messages in thread* [U-Boot] [PATCH 1/1] ubifs: avoid possible NULL dereference
2017-11-21 21:16 ` Wolfgang Denk
@ 2017-11-21 21:22 ` Ladislav Michl
2017-11-22 8:09 ` Wolfgang Denk
2017-11-21 21:28 ` Heinrich Schuchardt
1 sibling, 1 reply; 10+ messages in thread
From: Ladislav Michl @ 2017-11-21 21:22 UTC (permalink / raw)
To: u-boot
On Tue, Nov 21, 2017 at 10:16:40PM +0100, Wolfgang Denk wrote:
> Dear Heinrich,
>
> In message <20171121184503.3193-1-xypron.glpk@gmx.de> you wrote:
> > If 'file' cannot be allocated due to an out of memory
> > situation, do not dereference it.
> >
> > When debugging this patch also avoids a misleading message
> > "cannot find next direntry, error %d" in case of an out of
> > memory situation. It is sufficent to write
> > "%s: Error, no memory for malloc!\n" in this case.
> >
> > Reported-by: Alex Sadovsky <nable.maininbox@googlemail.com>
> > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > ---
> > fs/ubifs/ubifs.c | 5 ++---
> > 1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/ubifs/ubifs.c b/fs/ubifs/ubifs.c
> > index 4465523d5f..313dee0579 100644
> > --- a/fs/ubifs/ubifs.c
> > +++ b/fs/ubifs/ubifs.c
> > @@ -403,8 +403,7 @@ static int ubifs_finddir(struct super_block *sb, char *dirname,
> > dir = kzalloc(sizeof(struct inode), 0);
> > if (!file || !dentry || !dir) {
> > printf("%s: Error, no memory for malloc!\n", __func__);
> > - err = -ENOMEM;
> > - goto out;
> > + goto out_nomem;
> > }
> >
> > dir->i_sb = sb;
> > @@ -424,7 +423,6 @@ static int ubifs_finddir(struct super_block *sb, char *dirname,
> > err = PTR_ERR(dent);
> > goto out;
> > }
> > -
> > file->f_pos = key_hash_flash(c, &dent->key);
> > file->private_data = dent;
> >
> > @@ -463,6 +461,7 @@ out:
> >
> > out_free:
> > kfree(file->private_data);
> > +out_nomem:
> > free(file);
> > free(dentry);
> > free(dir);
>
> Should you not keep the "err = -ENOMEM;" setting? Otherwise there
> is no indivcation that an error happened.
It is not obvious from the patch, but value of err is later discarded.
It serves sole purpose of printing debug notice.
>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> There are no data that cannot be plotted on a straight line if the
> axis are chosen correctly.
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot
^ permalink raw reply [flat|nested] 10+ messages in thread* [U-Boot] [PATCH 1/1] ubifs: avoid possible NULL dereference
2017-11-21 21:22 ` Ladislav Michl
@ 2017-11-22 8:09 ` Wolfgang Denk
2017-11-22 8:25 ` Ladislav Michl
0 siblings, 1 reply; 10+ messages in thread
From: Wolfgang Denk @ 2017-11-22 8:09 UTC (permalink / raw)
To: u-boot
Dear Ladislav,
In message <20171121212222.ryicwv6tyh5rye2e@lenoch> you wrote:
> > >
> > > diff --git a/fs/ubifs/ubifs.c b/fs/ubifs/ubifs.c
> > > index 4465523d5f..313dee0579 100644
> > > --- a/fs/ubifs/ubifs.c
> > > +++ b/fs/ubifs/ubifs.c
> > > @@ -403,8 +403,7 @@ static int ubifs_finddir(struct super_block *sb, char *dirname,
> > > dir = kzalloc(sizeof(struct inode), 0);
> > > if (!file || !dentry || !dir) {
> > > printf("%s: Error, no memory for malloc!\n", __func__);
> > > - err = -ENOMEM;
> > > - goto out;
> > > + goto out_nomem;
> > > }
...
> > Should you not keep the "err = -ENOMEM;" setting? Otherwise there
> > is no indivcation that an error happened.
>
> It is not obvious from the patch, but value of err is later discarded.
> It serves sole purpose of printing debug notice.
So apparently we have a number of places in U-Boot where fatal
errors (running out of memory) are just ignored and we continue as
if nothing happened?
THis is short-sighted at best. One day Pump Six will fail.
This is giving me the creepes.
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Why don't you have a Linux partition installed so you can be working
in a programmer-friendly environment instead of a keep-gates'-bank-
account-happy one? :-) -- Tom Christiansen
^ permalink raw reply [flat|nested] 10+ messages in thread* [U-Boot] [PATCH 1/1] ubifs: avoid possible NULL dereference
2017-11-22 8:09 ` Wolfgang Denk
@ 2017-11-22 8:25 ` Ladislav Michl
2017-11-24 5:59 ` Heiko Schocher
0 siblings, 1 reply; 10+ messages in thread
From: Ladislav Michl @ 2017-11-22 8:25 UTC (permalink / raw)
To: u-boot
On Wed, Nov 22, 2017 at 09:09:36AM +0100, Wolfgang Denk wrote:
> Dear Ladislav,
>
> In message <20171121212222.ryicwv6tyh5rye2e@lenoch> you wrote:
> > > >
> > > > diff --git a/fs/ubifs/ubifs.c b/fs/ubifs/ubifs.c
> > > > index 4465523d5f..313dee0579 100644
> > > > --- a/fs/ubifs/ubifs.c
> > > > +++ b/fs/ubifs/ubifs.c
> > > > @@ -403,8 +403,7 @@ static int ubifs_finddir(struct super_block *sb, char *dirname,
> > > > dir = kzalloc(sizeof(struct inode), 0);
> > > > if (!file || !dentry || !dir) {
> > > > printf("%s: Error, no memory for malloc!\n", __func__);
> > > > - err = -ENOMEM;
> > > > - goto out;
> > > > + goto out_nomem;
> > > > }
> ...
> > > Should you not keep the "err = -ENOMEM;" setting? Otherwise there
> > > is no indivcation that an error happened.
> >
> > It is not obvious from the patch, but value of err is later discarded.
> > It serves sole purpose of printing debug notice.
>
> So apparently we have a number of places in U-Boot where fatal
> errors (running out of memory) are just ignored and we continue as
> if nothing happened?
While I have to admit this code is not an example of clean coding,
it prints notice when trying to manipulate with file.
fs/ubifs/ubifs.c as whole needs to be revisited, above patch just
caused shit hitting the fan.
> THis is short-sighted at best. One day Pump Six will fail.
>
> This is giving me the creepes.
I was just pointing to the fact, that above mentioned patch does not
make it any worse. Btw, initial commit is even more amazing:
+ if (file)
+ free(file);
+ if (dentry)
+ free(dentry);
+ if (dir)
+ free(dir);
+
+ if (file->private_data)
+ kfree(file->private_data);
+ file->private_data = NULL;
+ file->f_pos = 2;
+ return 0;
.oO(I guess it is safe not pointing where above snippet is comming from,
but will review whole file as I'm going to use it for new board I have to
suport)
Best regards,
ladis
^ permalink raw reply [flat|nested] 10+ messages in thread* [U-Boot] [PATCH 1/1] ubifs: avoid possible NULL dereference
2017-11-22 8:25 ` Ladislav Michl
@ 2017-11-24 5:59 ` Heiko Schocher
0 siblings, 0 replies; 10+ messages in thread
From: Heiko Schocher @ 2017-11-24 5:59 UTC (permalink / raw)
To: u-boot
Hello Ladislav,
Sorry for digging into it so late...
Am 22.11.2017 um 09:25 schrieb Ladislav Michl:
> On Wed, Nov 22, 2017 at 09:09:36AM +0100, Wolfgang Denk wrote:
>> Dear Ladislav,
>>
>> In message <20171121212222.ryicwv6tyh5rye2e@lenoch> you wrote:
>>>>>
>>>>> diff --git a/fs/ubifs/ubifs.c b/fs/ubifs/ubifs.c
>>>>> index 4465523d5f..313dee0579 100644
>>>>> --- a/fs/ubifs/ubifs.c
>>>>> +++ b/fs/ubifs/ubifs.c
>>>>> @@ -403,8 +403,7 @@ static int ubifs_finddir(struct super_block *sb, char *dirname,
>>>>> dir = kzalloc(sizeof(struct inode), 0);
>>>>> if (!file || !dentry || !dir) {
>>>>> printf("%s: Error, no memory for malloc!\n", __func__);
>>>>> - err = -ENOMEM;
>>>>> - goto out;
>>>>> + goto out_nomem;
>>>>> }
>> ...
>>>> Should you not keep the "err = -ENOMEM;" setting? Otherwise there
>>>> is no indivcation that an error happened.
>>>
>>> It is not obvious from the patch, but value of err is later discarded.
>>> It serves sole purpose of printing debug notice.
>>
>> So apparently we have a number of places in U-Boot where fatal
>> errors (running out of memory) are just ignored and we continue as
>> if nothing happened?
>
> While I have to admit this code is not an example of clean coding,
> it prints notice when trying to manipulate with file.
>
> fs/ubifs/ubifs.c as whole needs to be revisited, above patch just
> caused shit hitting the fan.
Yes, full ack ... we do need here a full review of this code.
>> THis is short-sighted at best. One day Pump Six will fail.
>>
>> This is giving me the creepes.
>
> I was just pointing to the fact, that above mentioned patch does not
> make it any worse. Btw, initial commit is even more amazing:
>
> + if (file)
> + free(file);
> + if (dentry)
> + free(dentry);
> + if (dir)
> + free(dir);
> +
> + if (file->private_data)
> + kfree(file->private_data);
> + file->private_data = NULL;
> + file->f_pos = 2;
> + return 0;
>
> .oO(I guess it is safe not pointing where above snippet is comming from,
> but will review whole file as I'm going to use it for new board I have to
> suport)
Many thanks!
bye,
Heiko
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52 Fax: +49-8142-66989-80 Email: hs at denx.de
^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH 1/1] ubifs: avoid possible NULL dereference
2017-11-21 21:16 ` Wolfgang Denk
2017-11-21 21:22 ` Ladislav Michl
@ 2017-11-21 21:28 ` Heinrich Schuchardt
1 sibling, 0 replies; 10+ messages in thread
From: Heinrich Schuchardt @ 2017-11-21 21:28 UTC (permalink / raw)
To: u-boot
On 11/21/2017 10:16 PM, Wolfgang Denk wrote:
> Dear Heinrich,
>
> In message <20171121184503.3193-1-xypron.glpk@gmx.de> you wrote:
>> If 'file' cannot be allocated due to an out of memory
>> situation, do not dereference it.
>>
>> When debugging this patch also avoids a misleading message
>> "cannot find next direntry, error %d" in case of an out of
>> memory situation. It is sufficent to write
>> "%s: Error, no memory for malloc!\n" in this case.
>>
>> Reported-by: Alex Sadovsky <nable.maininbox@googlemail.com>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>> fs/ubifs/ubifs.c | 5 ++---
>> 1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/ubifs/ubifs.c b/fs/ubifs/ubifs.c
>> index 4465523d5f..313dee0579 100644
>> --- a/fs/ubifs/ubifs.c
>> +++ b/fs/ubifs/ubifs.c
>> @@ -403,8 +403,7 @@ static int ubifs_finddir(struct super_block *sb, char *dirname,
>> dir = kzalloc(sizeof(struct inode), 0);
>> if (!file || !dentry || !dir) {
>> printf("%s: Error, no memory for malloc!\n", __func__);
>> - err = -ENOMEM;
>> - goto out;
>> + goto out_nomem;
>> }
>>
>> dir->i_sb = sb;
>> @@ -424,7 +423,6 @@ static int ubifs_finddir(struct super_block *sb, char *dirname,
>> err = PTR_ERR(dent);
>> goto out;
>> }
>> -
>> file->f_pos = key_hash_flash(c, &dent->key);
>> file->private_data = dent;
>>
>> @@ -463,6 +461,7 @@ out:
>>
>> out_free:
>> kfree(file->private_data);
>> +out_nomem:
>> free(file);
>> free(dentry);
>> free(dir);
>
> Should you not keep the "err = -ENOMEM;" setting? Otherwise there
> is no indivcation that an error happened.
err is a local variable it is not returned by the function. It is only
used to show a debug message which does not make sense in this case.
The "inidcation" is returning 0 and
printf("%s: Error, no memory for malloc!\n", __func__)
Best regards
Heinrich
>
> Best regards,
>
> Wolfgang Denk
>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-11-24 5:59 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-11-21 18:45 [U-Boot] [PATCH 1/1] ubifs: avoid possible NULL dereference Heinrich Schuchardt
2017-11-21 20:23 ` Ladislav Michl
2017-11-21 21:29 ` Heinrich Schuchardt
2017-11-21 21:55 ` Ladislav Michl
2017-11-21 21:16 ` Wolfgang Denk
2017-11-21 21:22 ` Ladislav Michl
2017-11-22 8:09 ` Wolfgang Denk
2017-11-22 8:25 ` Ladislav Michl
2017-11-24 5:59 ` Heiko Schocher
2017-11-21 21:28 ` Heinrich Schuchardt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox