[PATCH] af_unix: Don't set err in unix_stream_read_generic unless there was an error

Subsystems: networking [general], networking [unix sockets], the rest

STALE3832d

3 messages, 3 authors, 2016-02-13 · open the first message on its own page

[PATCH] af_unix: Don't set err in unix_stream_read_generic unless there was an error

From: Rainer Weikusat <hidden>
Date: 2016-02-05 22:30:21

The present unix_stream_read_generic contains various code sequences of
the form

err = -EDISASTER;
if (<test>)
	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.

Signed-off-by: Rainer Weikusat <redacted>
---

With proper subject this time (at least I hope so).
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 49d5093..138787d 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -2277,13 +2277,15 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state)
 	size_t size = state->size;
 	unsigned int last_len;
 
-	err = -EINVAL;
-	if (sk->sk_state != TCP_ESTABLISHED)
+	if (sk->sk_state != TCP_ESTABLISHED) {
+		err = -EINVAL;
 		goto out;
+	}
 
-	err = -EOPNOTSUPP;
-	if (flags & MSG_OOB)
+	if (flags & MSG_OOB) {
+		err = -EOPNOTSUPP;
 		goto out;
+	}
 
 	target = sock_rcvlowat(sk, flags & MSG_WAITALL, size);
 	timeo = sock_rcvtimeo(sk, noblock);
@@ -2329,9 +2331,11 @@ again:
 				goto unlock;
 
 			unix_state_unlock(sk);
-			err = -EAGAIN;
-			if (!timeo)
+			if (!timeo) {
+				err = -EAGAIN;
 				break;
+			}
+
 			mutex_unlock(&u->readlock);
 
 			timeo = unix_stream_data_wait(sk, timeo, last,

Re: [PATCH] af_unix: Don't set err in unix_stream_read_generic unless there was an error

From: Joseph Salisbury <hidden>
Date: 2016-02-11 21:32:00

On 02/05/2016 05:30 PM, Rainer Weikusat wrote:
quoted hunk
The present unix_stream_read_generic contains various code sequences of
the form

err = -EDISASTER;
if (<test>)
	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.

Signed-off-by: Rainer Weikusat <redacted>
---

With proper subject this time (at least I hope so).
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 49d5093..138787d 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -2277,13 +2277,15 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state)
 	size_t size = state->size;
 	unsigned int last_len;
 
-	err = -EINVAL;
-	if (sk->sk_state != TCP_ESTABLISHED)
+	if (sk->sk_state != TCP_ESTABLISHED) {
+		err = -EINVAL;
 		goto out;
+	}
 
-	err = -EOPNOTSUPP;
-	if (flags & MSG_OOB)
+	if (flags & MSG_OOB) {
+		err = -EOPNOTSUPP;
 		goto out;
+	}
 
 	target = sock_rcvlowat(sk, flags & MSG_WAITALL, size);
 	timeo = sock_rcvtimeo(sk, noblock);
@@ -2329,9 +2331,11 @@ again:
 				goto unlock;
 
 			unix_state_unlock(sk);
-			err = -EAGAIN;
-			if (!timeo)
+			if (!timeo) {
+				err = -EAGAIN;
 				break;
+			}
+
 			mutex_unlock(&u->readlock);
 
 			timeo = unix_stream_data_wait(sk, timeo, last,
I tested your patch, Rainer.  I can confirm that it fixes the reported
bug[0].

Thanks for the quick response!

Joe

[0] http://pad.lv/1540731

Re: [PATCH] af_unix: Don't set err in unix_stream_read_generic unless there was an error

From: Ben Hutchings <hidden>
Date: 2016-02-13 00:19:28

On Fri, 2016-02-05 at 22:30 +0000, Rainer Weikusat wrote:
The present unix_stream_read_generic contains various code sequences of
the form

err = -EDISASTER;
if ()
	goto out;
[...]

I wish people would stop writing code like this.  At one time it may
have been a useful micro-optimisation, avoiding an extra branch in the
successful case, but gcc now appears to do that itself.  So it makes
the code less clear and runs the risk of introducing this sort of bug,
for no obvious benefit.

Ben.

-- 
Ben Hutchings
Sturgeon's Law: Ninety percent of everything is crap.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help