linux-kernel - Re: [PATCH] af_unix: Don't set err in unix_stream_read

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1455668691.1143.18.camel@decadent.org.uk>
Date:	Wed, 17 Feb 2016 00:24:51 +0000
From:	Ben Hutchings <ben@...adent.org.uk>
To:	David Miller <davem@...emloft.net>,
	rweikusat@...ileactivedefense.com
Cc:	hannes@...essinduktion.org, edumazet@...gle.com,
	dhowells@...hat.com, ying.xue@...driver.com,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	stable@...r.kernel.org, joseph.salisbury@...onical.com
Subject: Re: [PATCH] af_unix: Don't set err in unix_stream_read_generic
 unless there was an error

On Tue, 2016-02-16 at 12:51 -0500, David Miller wrote:
> From: Rainer Weikusat <rweikusat@...ileactivedefense.com>
> Date: Mon, 08 Feb 2016 18:47:19 +0000
> 
> > The present unix_stream_read_generic contains various code sequences of
> > the form
> > 
> > err = -EDISASTER;
> > if ()
> >       goto out;
> > 
> > This has the unfortunate side effect of possibly causing the error code
> > to bleed through to the final
> > 
> > out:
> >       return copied ? : err;
> > 
> > and then to be wrongly returned if no data was copied because the caller
> > didn't supply a data buffer, as demonstrated by the program available at
> > 
> > http://pad.lv/1540731
> > 
> > Change it such that err is only set if an error condition was detected.
> > 
> > Fixes: 3822b5c2fc62 ("af_unix: Revert 'lock_interruptible' in stream receive code")
> > Reported-by: Joseph Salisbury <joseph.salisbury@...onical.com>
> > Signed-off-by: Rainer Weikusat <rweikusat@...ileactivedefense.com>
> 
> Applied, thanks Rainer.
> 
> And BTW I disagree with some of the feedback I saw in these threads
> about "if (x) goto out;" being unreadable and that it should be avoided.

That's not what I said.

> That's completely wrong.
> 
> Fact is, we've all been reading code of that form for multiple decades.
> So it's the style we are _MOST_ familiar with, and it is therefore the
> style that is the easiest and clearest for kernel developers to understand.
[...]

I agree that 'if (err) goto cleanup;' is widely used and is generally
understandable (though more creative uses of goto are often not).

My objection was to 'err = -EFOO; if (cond) goto cleanup;'.  That is definitely not clear and it hides mistakes like this.

Ben.

-- 
Ben Hutchings
Lowery's Law:
             If it jams, force it. If it breaks, it needed replacing anyway.
Download attachment "signature.asc" of type "application/pgp-signature" (812 bytes)