[PATCH] Check for Null return of function of affs_bread in function affs_truncate

[PATCH] Check for Null return of function of affs_bread in function affs_truncate

[Nicholas Krause]

Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
---
 fs/affs/file.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/affs/file.c b/fs/affs/file.c
index a7fe57d..f26482d 100644
--- a/fs/affs/file.c
+++ b/fs/affs/file.c
@@ -923,6 +923,8 @@ affs_truncate(struct inode *inode)
 
 	while (ext_key) {
 		ext_bh = affs_bread(sb, ext_key);
+		if (!ext_bh)
+			return;
 		size = AFFS_SB(sb)->s_hashsize;
 		if (size > blkcnt - blk)
 			size = blkcnt - blk;
-- 
1.9.1

Re: [PATCH] Check for Null return of function of affs_bread in function affs_truncate

[Dan Carpenter]

On Wed, Jun 18, 2014 at 06:08:05PM -0400, Nicholas Krause wrote:
> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
> ---
>  fs/affs/file.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/affs/file.c b/fs/affs/file.c
> index a7fe57d..f26482d 100644
> --- a/fs/affs/file.c
> +++ b/fs/affs/file.c
> @@ -923,6 +923,8 @@ affs_truncate(struct inode *inode)
>  
>  	while (ext_key) {
>  		ext_bh = affs_bread(sb, ext_key);
> +		if (!ext_bh)
> +			return;

The problem is that we don't know if we should return here or break
here.  If you don't understand the code, then it's best to just leave it
alone.

regards,
dan carpenter

>  		size = AFFS_SB(sb)->s_hashsize;
>  		if (size > blkcnt - blk)
>  			size = blkcnt - blk;
> -- 
> 1.9.1

Re: [PATCH] Check for Null return of function of affs_bread in function affs_truncate

[Nick Krause]

Ok that's fine I would return as if it's a NULL the other parts of the
function can't continue.
Nick

On Thu, Jun 19, 2014 at 1:21 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Wed, Jun 18, 2014 at 06:08:05PM -0400, Nicholas Krause wrote:
>> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
>> ---
>>  fs/affs/file.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/fs/affs/file.c b/fs/affs/file.c
>> index a7fe57d..f26482d 100644
>> --- a/fs/affs/file.c
>> +++ b/fs/affs/file.c
>> @@ -923,6 +923,8 @@ affs_truncate(struct inode *inode)
>>
>>       while (ext_key) {
>>               ext_bh = affs_bread(sb, ext_key);
>> +             if (!ext_bh)
>> +                     return;
>
> The problem is that we don't know if we should return here or break
> here.  If you don't understand the code, then it's best to just leave it
> alone.
>
> regards,
> dan carpenter
>
>>               size = AFFS_SB(sb)->s_hashsize;
>>               if (size > blkcnt - blk)
>>                       size = blkcnt - blk;
>> --
>> 1.9.1

Re: [PATCH] Check for Null return of function of affs_bread in function affs_truncate

[Thomas Gleixner]

On Fri, 20 Jun 2014, Nick Krause wrote:

> Ok that's fine I would return as if it's a NULL the other parts of the
> function can't continue.
> Nick
> 
> On Thu, Jun 19, 2014 at 1:21 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > On Wed, Jun 18, 2014 at 06:08:05PM -0400, Nicholas Krause wrote:
> >> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
> >> ---
> >>  fs/affs/file.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/fs/affs/file.c b/fs/affs/file.c
> >> index a7fe57d..f26482d 100644
> >> --- a/fs/affs/file.c
> >> +++ b/fs/affs/file.c
> >> @@ -923,6 +923,8 @@ affs_truncate(struct inode *inode)
> >>
> >>       while (ext_key) {
> >>               ext_bh = affs_bread(sb, ext_key);
> >> +             if (!ext_bh)
> >> +                     return;
> >
> > The problem is that we don't know if we should return here or break
> > here.  If you don't understand the code, then it's best to just leave it
> > alone.

Dan, what kind of attitude is that?

Nick certainly found an issue where a possible NULL return from
affs_bread() can cause havoc.

Do YOU understand that code?

If yes, you better explain, WHY Nicks finding is a false positive
instead of just telling him off in a very inpolite way.

If not, you better refrain from telling a reporter that he does not
understand the code and should stay away.

You clearly stated that you do not understand it either:

> > The problem is that we don't know if we should return here or break
> > here.

The problem here is that proceeding with a known NULL pointer is wrong
to begin with. It does not matter at all whether break or return is
the proper thing to do. What matters is that proceeding with a NULL
pointer is wrong to begin with, no matter what.

So either explain why this is a non issue and the NULL pointer return
cannot happen or shut up and try to find a proper solution for that
"return" vs. "break" issue.

Thanks,

	tglx

Re: [PATCH] Check for Null return of function of affs_bread in function affs_truncate

[Nick Krause]

Thanks for standing up for me Thomas.
If you have any ideas about what is better
please let me known.
Cheers Nick

On Fri, Jun 20, 2014 at 7:59 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Fri, 20 Jun 2014, Nick Krause wrote:
>
>> Ok that's fine I would return as if it's a NULL the other parts of the
>> function can't continue.
>> Nick
>>
>> On Thu, Jun 19, 2014 at 1:21 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>> > On Wed, Jun 18, 2014 at 06:08:05PM -0400, Nicholas Krause wrote:
>> >> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
>> >> ---
>> >>  fs/affs/file.c | 2 ++
>> >>  1 file changed, 2 insertions(+)
>> >>
>> >> diff --git a/fs/affs/file.c b/fs/affs/file.c
>> >> index a7fe57d..f26482d 100644
>> >> --- a/fs/affs/file.c
>> >> +++ b/fs/affs/file.c
>> >> @@ -923,6 +923,8 @@ affs_truncate(struct inode *inode)
>> >>
>> >>       while (ext_key) {
>> >>               ext_bh = affs_bread(sb, ext_key);
>> >> +             if (!ext_bh)
>> >> +                     return;
>> >
>> > The problem is that we don't know if we should return here or break
>> > here.  If you don't understand the code, then it's best to just leave it
>> > alone.
>
> Dan, what kind of attitude is that?
>
> Nick certainly found an issue where a possible NULL return from
> affs_bread() can cause havoc.
>
> Do YOU understand that code?
>
> If yes, you better explain, WHY Nicks finding is a false positive
> instead of just telling him off in a very inpolite way.
>
> If not, you better refrain from telling a reporter that he does not
> understand the code and should stay away.
>
> You clearly stated that you do not understand it either:
>
>> > The problem is that we don't know if we should return here or break
>> > here.
>
> The problem here is that proceeding with a known NULL pointer is wrong
> to begin with. It does not matter at all whether break or return is
> the proper thing to do. What matters is that proceeding with a NULL
> pointer is wrong to begin with, no matter what.
>
> So either explain why this is a non issue and the NULL pointer return
> cannot happen or shut up and try to find a proper solution for that
> "return" vs. "break" issue.
>
> Thanks,
>
>         tglx

Re: [PATCH] Check for Null return of function of affs_bread in function affs_truncate

[Andrew Morton]

On Fri, 20 Jun 2014 22:25:47 -0400 Nick Krause <xerofoify@gmail.com> wrote:

> If you have any ideas about what is better
> please let me known.

I think the proposed patch was not a good one - it will cause truncate
to silently return, probably leaving the fs in an inconsistent state. 
Neither the user nor the running application know this happened so they
will just keep on modifying the filesystem, possibly mangling it
further.

The code as it stands at present is better - if bread() fails we'll get
a nice solid oops and the current app will be terminated (at least). 
As we're in truncate it's quite possible that the entire fs will get
wedged up due to now-permanently-held i_mutex, which is even better.


As for the best fix, umm, hard.  We're pretty screwed if we cannot read
that block at this code site.  Perhaps emit loud printks, forcibly turn
the fs read-only then return -EIO/-ENOMEM/etc from the truncate.  Such
a change would require runtime testing, with some form of developer fault
injection.

Re: [PATCH] Check for Null return of function of affs_bread in function affs_truncate

[Nick Krause]

Fair enough if somebody is running this file system I would be
happy to have someone test my code in order to fix this.
Cheers Nick

On Fri, Jun 20, 2014 at 10:38 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Fri, 20 Jun 2014 22:25:47 -0400 Nick Krause <xerofoify@gmail.com> wrote:
>
>> If you have any ideas about what is better
>> please let me known.
>
> I think the proposed patch was not a good one - it will cause truncate
> to silently return, probably leaving the fs in an inconsistent state.
> Neither the user nor the running application know this happened so they
> will just keep on modifying the filesystem, possibly mangling it
> further.
>
> The code as it stands at present is better - if bread() fails we'll get
> a nice solid oops and the current app will be terminated (at least).
> As we're in truncate it's quite possible that the entire fs will get
> wedged up due to now-permanently-held i_mutex, which is even better.
>
>
> As for the best fix, umm, hard.  We're pretty screwed if we cannot read
> that block at this code site.  Perhaps emit loud printks, forcibly turn
> the fs read-only then return -EIO/-ENOMEM/etc from the truncate.  Such
> a change would require runtime testing, with some form of developer fault
> injection.
>
>
>

Re: [PATCH] Check for Null return of function of affs_bread in function affs_truncate

[Andrew Morton]

On Fri, 20 Jun 2014 22:55:07 -0400 Nick Krause <xerofoify@gmail.com> wrote:

> On Fri, Jun 20, 2014 at 10:38 PM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
> > On Fri, 20 Jun 2014 22:25:47 -0400 Nick Krause <xerofoify@gmail.com> wrote:
> >
> >> If you have any ideas about what is better
> >> please let me known.
> >
> > I think the proposed patch was not a good one - it will cause truncate
> > to silently return, probably leaving the fs in an inconsistent state.
> > Neither the user nor the running application know this happened so they
> > will just keep on modifying the filesystem, possibly mangling it
> > further.
> >
> > The code as it stands at present is better - if bread() fails we'll get
> > a nice solid oops and the current app will be terminated (at least).
> > As we're in truncate it's quite possible that the entire fs will get
> > wedged up due to now-permanently-held i_mutex, which is even better.
> >
> >
> > As for the best fix, umm, hard.  We're pretty screwed if we cannot read
> > that block at this code site.  Perhaps emit loud printks, forcibly turn
> > the fs read-only then return -EIO/-ENOMEM/etc from the truncate.  Such
> > a change would require runtime testing, with some form of developer fault
> > injection.
>
> Fair enough if somebody is running this file system I would be
> happy to have someone test my code in order to fix this.

(top-posting repaired - please don't top-post!)

It's going to be hard to find such a person.  As mkfs.affs doesn't
appear to exist (?) your best bet would be to find someone who has an
Amiga, get them to create a new fs for you (via loopback-on-file) then
gzip the underlying file and send it to you.  You can then use that fs
image file as many times as you want via loopback or straight onto a
disk.  Make sure the image file is zeroed out first so it compresses
well.

Or something like that.

Re: [PATCH] Check for Null return of function of affs_bread in function affs_truncate

[Nick Krause]

I don't think that it's a good idea , in that case I
would recommend either leaving this bug open
or close it as there doesn't seem to be a good
way of testing this.
Cheers Nick

On Fri, Jun 20, 2014 at 11:09 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Fri, 20 Jun 2014 22:55:07 -0400 Nick Krause <xerofoify@gmail.com> wrote:
>
>> On Fri, Jun 20, 2014 at 10:38 PM, Andrew Morton
>> <akpm@linux-foundation.org> wrote:
>> > On Fri, 20 Jun 2014 22:25:47 -0400 Nick Krause <xerofoify@gmail.com> wrote:
>> >
>> >> If you have any ideas about what is better
>> >> please let me known.
>> >
>> > I think the proposed patch was not a good one - it will cause truncate
>> > to silently return, probably leaving the fs in an inconsistent state.
>> > Neither the user nor the running application know this happened so they
>> > will just keep on modifying the filesystem, possibly mangling it
>> > further.
>> >
>> > The code as it stands at present is better - if bread() fails we'll get
>> > a nice solid oops and the current app will be terminated (at least).
>> > As we're in truncate it's quite possible that the entire fs will get
>> > wedged up due to now-permanently-held i_mutex, which is even better.
>> >
>> >
>> > As for the best fix, umm, hard.  We're pretty screwed if we cannot read
>> > that block at this code site.  Perhaps emit loud printks, forcibly turn
>> > the fs read-only then return -EIO/-ENOMEM/etc from the truncate.  Such
>> > a change would require runtime testing, with some form of developer fault
>> > injection.
>>
>> Fair enough if somebody is running this file system I would be
>> happy to have someone test my code in order to fix this.
>
> (top-posting repaired - please don't top-post!)
>
> It's going to be hard to find such a person.  As mkfs.affs doesn't
> appear to exist (?) your best bet would be to find someone who has an
> Amiga, get them to create a new fs for you (via loopback-on-file) then
> gzip the underlying file and send it to you.  You can then use that fs
> image file as many times as you want via loopback or straight onto a
> disk.  Make sure the image file is zeroed out first so it compresses
> well.
>
> Or something like that.

Re: [PATCH] Check for Null return of function of affs_bread in function affs_truncate

[Al Viro]

On Sat, Jun 21, 2014 at 01:59:15AM +0200, Thomas Gleixner wrote:
> > > The problem is that we don't know if we should return here or break
> > > here.  If you don't understand the code, then it's best to just leave it
> > > alone.
> 
> Dan, what kind of attitude is that?
>
> Nick certainly found an issue where a possible NULL return from
> affs_bread() can cause havoc.

Yes.  No arguments here.

> Do YOU understand that code?
> 
> If yes, you better explain, WHY Nicks finding is a false positive
> instead of just telling him off in a very inpolite way.

It's not a false positive at all.

> If not, you better refrain from telling a reporter that he does not
> understand the code and should stay away.

Tone aside, he does have a point - namely, that patch in question doesn't
contain any analysis of the bug and recovery strategy, turning a bug into
something that is much harder to spot on inspection.

If nothing else, such patches should contain a loud printk added on
the b0rken codepath, along with a big fat warning in the source.

I'm not saying that "fuck off unless you understand the code" is a sane
policy - it's not.  But "anything's better than an oops" is also wrong.

> > > The problem is that we don't know if we should return here or break
> > > here.
> 
> The problem here is that proceeding with a known NULL pointer is wrong
> to begin with. It does not matter at all whether break or return is
> the proper thing to do. What matters is that proceeding with a NULL
> pointer is wrong to begin with, no matter what.
> 
> So either explain why this is a non issue and the NULL pointer return
> cannot happen or shut up and try to find a proper solution for that
> "return" vs. "break" issue.

return vs. break here is the difference between discarding preallocated blocks
and leaking them as well.  The thing is, it's either severe OOM or a corrupted
image.  I'd *probably* go for "affs_error() and return" here, but that's
not the only question - we probably ought to make it return an error, instead
of having it void.  And callers tend to do that affs_free_prealloc(), so
much that I'm not sure if we actually want to keep it in affs_truncate() at
all.

Re: [PATCH] Check for Null return of function of affs_bread in function affs_truncate

[Dan Carpenter]

On Sat, Jun 21, 2014 at 01:59:15AM +0200, Thomas Gleixner wrote:
> On Fri, 20 Jun 2014, Nick Krause wrote:
> 
> > Ok that's fine I would return as if it's a NULL the other parts of the
> > function can't continue.
> > Nick
> > 
> > On Thu, Jun 19, 2014 at 1:21 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > On Wed, Jun 18, 2014 at 06:08:05PM -0400, Nicholas Krause wrote:
> > >> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
> > >> ---
> > >>  fs/affs/file.c | 2 ++
> > >>  1 file changed, 2 insertions(+)
> > >>
> > >> diff --git a/fs/affs/file.c b/fs/affs/file.c
> > >> index a7fe57d..f26482d 100644
> > >> --- a/fs/affs/file.c
> > >> +++ b/fs/affs/file.c
> > >> @@ -923,6 +923,8 @@ affs_truncate(struct inode *inode)
> > >>
> > >>       while (ext_key) {
> > >>               ext_bh = affs_bread(sb, ext_key);
> > >> +             if (!ext_bh)
> > >> +                     return;
> > >
> > > The problem is that we don't know if we should return here or break
> > > here.  If you don't understand the code, then it's best to just leave it
> > > alone.
> 
> Dan, what kind of attitude is that?

I'm just catching up on email after being offline for a while.

I apologize that my email came off ruder than intended.

I just meant that as a general rule, sometimes you should leave the
static checker warning there if you aren't sure what the correct fix is.
Even when it's a real bug, don't just guess at it, you have to be sure.
Otherwise you just create a more subtle bug that the static checker
can't detect.

regards,
dan carpenter

Re: [PATCH] Check for Null return of function of affs_bread in function affs_truncate

[Nick Krause]

On Fri, Jul 11, 2014 at 11:05 AM, Dan Carpenter
<dan.carpenter@oracle.com> wrote:
> On Sat, Jun 21, 2014 at 01:59:15AM +0200, Thomas Gleixner wrote:
>> On Fri, 20 Jun 2014, Nick Krause wrote:
>>
>> > Ok that's fine I would return as if it's a NULL the other parts of the
>> > function can't continue.
>> > Nick
>> >
>> > On Thu, Jun 19, 2014 at 1:21 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>> > > On Wed, Jun 18, 2014 at 06:08:05PM -0400, Nicholas Krause wrote:
>> > >> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
>> > >> ---
>> > >>  fs/affs/file.c | 2 ++
>> > >>  1 file changed, 2 insertions(+)
>> > >>
>> > >> diff --git a/fs/affs/file.c b/fs/affs/file.c
>> > >> index a7fe57d..f26482d 100644
>> > >> --- a/fs/affs/file.c
>> > >> +++ b/fs/affs/file.c
>> > >> @@ -923,6 +923,8 @@ affs_truncate(struct inode *inode)
>> > >>
>> > >>       while (ext_key) {
>> > >>               ext_bh = affs_bread(sb, ext_key);
>> > >> +             if (!ext_bh)
>> > >> +                     return;
>> > >
>> > > The problem is that we don't know if we should return here or break
>> > > here.  If you don't understand the code, then it's best to just leave it
>> > > alone.
>>
>> Dan, what kind of attitude is that?
>
> I'm just catching up on email after being offline for a while.
>
> I apologize that my email came off ruder than intended.
>
> I just meant that as a general rule, sometimes you should leave the
> static checker warning there if you aren't sure what the correct fix is.
> Even when it's a real bug, don't just guess at it, you have to be sure.
> Otherwise you just create a more subtle bug that the static checker
> can't detect.
>
> regards,
> dan carpenter
>
Hey Dan,
I am pretty sure this is correct as affs_bread does not check for NULL
and I am checking
the return of this function. If you feel this patch is wrong and would
like me to rewrite it.
Cheers Nick