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

[Al Viro <viro@ZenIV.linux.org.uk>]

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.