AF_UNIX, accept() and addrlen

PATCH In review Samuel Thibault 5 participants 17 messages Mar 8, 2008 – May 12, 2008

Subsystems (2)

Networking Networking unix sockets

lore

Share & tools

lore

Samuel Thibault

author

2008-03-08 02:23 UTC #0 lore raw

PATCH 1/1 AF_UNIX, accept() and addrlen

Hello,

Accept and getpeername are supposed to return the amount of bytes written in the returned address. However, on unnamed sockets, only sizeof(short) is returned, while a 0 is put in the sun_path member. This patch adds 1 for that additional byte.

Signed-off-by Samuel Thibault <[email protected]>

net/unix/af_unix.c renamed from net/unix/af_unix.c.orig +1 -1

@@ -1274,7 +1274,7 @@ static int unix_getname(struct socket *s
 	if (!u->addr) {
 		sunaddr->sun_family = AF_UNIX;
 		sunaddr->sun_path[0] = 0;
-		*uaddr_len = sizeof(short);
+		*uaddr_len = sizeof(short) + 1;
 	} else {
 		struct unix_address *addr = u->addr;

David Miller

maintainer

2008-03-24 04:56 UTC #1 lore raw

From: Samuel Thibault <[email protected]>
Date: Sat, 8 Mar 2008 02:23:21 +0000

Quoted 6 lines

Accept and getpeername are supposed to return the amount of bytes written in the returned address. However, on unnamed sockets, only sizeof(short) is returned, while a 0 is put in the sun_path member. This patch adds 1 for that additional byte.
Signed-off-by Samuel Thibault <[email protected]>

This change isn't correct. It's the fact that the length returned is sizeof(short) that tells the caller that the unix socket is unnamed.

We zero out the sun_path[0] member just to be polite and tidy.

You would break applications if you changed this, so marking this patch as "trivial" is extremely premature.

Samuel Thibault

author

2008-03-24 10:44 UTC #2 lore raw

Quoted 8 lines · David Miller, le Sun 23 Mar 2008 21:56:41 -0700, a écrit :

From: Samuel Thibault <[email protected]>
Date: Sat, 8 Mar 2008 02:23:21 +0000
Quoted 5 lines
Accept and getpeername are supposed to return the amount of bytes written in the returned address. However, on unnamed sockets, only sizeof(short) is returned, while a 0 is put in the sun_path member. This patch adds 1 for that additional byte.

Quoted 5 lines

Quoted 1 line
Signed-off-by Samuel Thibault <[email protected]>
This change isn't correct. It's the fact that the length returned is sizeof(short) that tells the caller that the unix socket is unnamed.

Mmm, where that is documented?

I can't find any details about that in SUS, and man 7 unix says

`If sun_path starts with a null byte ('' '), then it refers to the abstract namespace main- tained by the Unix protocol module.'

It doesn't talk about the size being only sizeof(short) (which I guess you meant sizeof(sa_family_t) actually).

Quoted 4 lines

We zero out the sun_path[0] member just to be polite and tidy.
You would break applications if you changed this, so marking this patch as "trivial" is extremely premature.

See documentation above. If applications don't follow documentation, then they deserve breaking :)

Note also that on some (BSD-ish) systems, sockaddr_un contains a sun_len field, containing the length of the data, and thus on them accept and getpeername return more that sizeof(sa_family_t) as length (it actually returns 16). So such applications are really broken.

Samuel

Andi Kleen

2008-03-24 11:50 UTC #3 lore raw

Quoted 15 lines · Samuel Thibault

Quoted 8 lines · David Miller, le Sun 23 Mar 2008 21:56:41 -0700, a écrit :
From: Samuel Thibault <[email protected]>
Date: Sat, 8 Mar 2008 02:23:21 +0000
Quoted 5 lines
Accept and getpeername are supposed to return the amount of bytes written in the returned address. However, on unnamed sockets, only sizeof(short) is returned, while a 0 is put in the sun_path member. This patch adds 1 for that additional byte.
Quoted 5 lines
Quoted 1 line
Signed-off-by Samuel Thibault <[email protected]>
This change isn't correct. It's the fact that the length returned is sizeof(short) that tells the caller that the unix socket is unnamed.

Quoted 6 lines

Mmm, where that is documented?
I can't find any details about that in SUS, and man 7 unix says
`If sun_path starts with a null byte ('' '), then it refers to the abstract namespace main- tained by the Unix protocol module.'

[I wrote unix(7) originally]. The abstract name space is a Linux extension and there is no written standard and whatever the kernel implements is the de-facto standard. If unix(7) differs in anything from what the code does please send patches to the manpages maintainer.

-Andi

Samuel Thibault

author

2008-03-24 12:18 UTC #4 lore raw

Quoted 21 lines · Andi Kleen, le Mon 24 Mar 2008 12:50:10 +0100, a écrit :

Quoted 19 lines · Samuel Thibault
Quoted 8 lines · David Miller, le Sun 23 Mar 2008 21:56:41 -0700, a écrit :
From: Samuel Thibault <[email protected]>
Date: Sat, 8 Mar 2008 02:23:21 +0000
Quoted 5 lines
Accept and getpeername are supposed to return the amount of bytes written in the returned address. However, on unnamed sockets, only sizeof(short) is returned, while a 0 is put in the sun_path member. This patch adds 1 for that additional byte.
Quoted 5 lines
Quoted 1 line
Signed-off-by Samuel Thibault <[email protected]>
This change isn't correct. It's the fact that the length returned is sizeof(short) that tells the caller that the unix socket is unnamed.
Mmm, where that is documented?
I can't find any details about that in SUS, and man 7 unix says

Quoted 8 lines

Quoted 2 lines
`If sun_path starts with a null byte ('' '), then it refers to the abstract namespace main- tained by the Unix protocol module.'
[I wrote unix(7) originally]. The abstract name space is a Linux extension and there is no written standard and whatever the kernel implements is the de-facto standard. If unix(7) differs in anything from what the code does please send patches to the manpages maintainer.

Oops, sorry, we are not talking about abstract namespace actually (their sockaddr length are necessarily bigger than sizeof(sa_family_t) since they need some data), but unamed sockets. So the Address Format paragraph just misses description of unnamed sockets.

Samuel

Samuel Thibault

author

2008-03-24 12:27 UTC #5 lore raw

Quoted 30 lines · Samuel Thibault, le Mon 24 Mar 2008 12:17:19 +0000, a écrit :

Quoted 20 lines · Andi Kleen, le Mon 24 Mar 2008 12:50:10 +0100, a écrit :
Quoted 19 lines · Samuel Thibault
Quoted 8 lines · David Miller, le Sun 23 Mar 2008 21:56:41 -0700, a écrit :
From: Samuel Thibault <[email protected]>
Date: Sat, 8 Mar 2008 02:23:21 +0000
Quoted 5 lines
Accept and getpeername are supposed to return the amount of bytes written in the returned address. However, on unnamed sockets, only sizeof(short) is returned, while a 0 is put in the sun_path member. This patch adds 1 for that additional byte.
Quoted 5 lines
Quoted 1 line
Signed-off-by Samuel Thibault <[email protected]>
This change isn't correct. It's the fact that the length returned is sizeof(short) that tells the caller that the unix socket is unnamed.
Mmm, where that is documented?
I can't find any details about that in SUS, and man 7 unix says
Quoted 8 lines
Quoted 2 lines
`If sun_path starts with a null byte ('' '), then it refers to the abstract namespace main- tained by the Unix protocol module.'
[I wrote unix(7) originally]. The abstract name space is a Linux extension and there is no written standard and whatever the kernel implements is the de-facto standard. If unix(7) differs in anything from what the code does please send patches to the manpages maintainer.

Quoted 4 lines

Oops, sorry, we are not talking about abstract namespace actually (their sockaddr length are necessarily bigger than sizeof(sa_family_t) since they need some data), but unamed sockets. So the Address Format paragraph just misses description of unnamed sockets.

How about this?

unix.7 renamed from unix.7.orig +9 -0

@@ -87,6 +87,15 @@
 bytes in
 .IR sun_path .
 Note that names in the abstract namespace are not zero-terminated.
+If the size returned by
+.BR accept
+or
+.BR getpeername
+is
+.IR sizeof(sa_family_t) ,
+then it refers to a unnamed socket and
+.I sun_path
+should not be read.
 .SS Socket Options
 For historical reasons these socket options are specified with a
 .B SOL_SOCKET

Samuel

Michael Kerrisk

2008-03-31 04:01 UTC #6 lore raw

Samuel,

It would really be much more useful if you CCed me, rather than hoping that I'd find this patch by trawling LKML...

David, Andi,

My understanding about abstract namespace sockets (what Samuel calls unnamed sockets) is that the indicator that the address is for an unnamed socket is that the sun_path starts with a zero byte -- and the *entire* remainder of the sun_path constitutes the name of the socket.
As such, information about the size returned by accept() etc. is
redundant. (I've happily written programs that use abstract namespace sockets without even knowing what is returned by a succesful accept().)

I agree with Samuel that there should be some documentation of the return value of accept() etc, for abstract sockets but my inclination would be to document that the indicator that this is an abstract socket is the initial null byte in sun_path, and mention the returned length as an after word. Does this seem reasonable?

Cheers,

Michael

Quoted 67 lines · Samuel Thibault

Quoted 30 lines · Samuel Thibault, le Mon 24 Mar 2008 12:17:19 +0000, a écrit :
Quoted 20 lines · Andi Kleen, le Mon 24 Mar 2008 12:50:10 +0100, a écrit :
Quoted 19 lines · Samuel Thibault
Quoted 8 lines · David Miller, le Sun 23 Mar 2008 21:56:41 -0700, a écrit :
From: Samuel Thibault <[email protected]>
Date: Sat, 8 Mar 2008 02:23:21 +0000
Quoted 5 lines
Accept and getpeername are supposed to return the amount of bytes written in the returned address. However, on unnamed sockets, only sizeof(short) is returned, while a 0 is put in the sun_path member. This patch adds 1 for that additional byte.
Quoted 5 lines
Quoted 1 line
Signed-off-by Samuel Thibault <[email protected]>
This change isn't correct. It's the fact that the length returned is sizeof(short) that tells the caller that the unix socket is unnamed.
Mmm, where that is documented?
I can't find any details about that in SUS, and man 7 unix says
Quoted 8 lines
Quoted 2 lines
`If sun_path starts with a null byte ('' '), then it refers to the abstract namespace main- tained by the Unix protocol module.'
[I wrote unix(7) originally]. The abstract name space is a Linux extension and there is no written standard and whatever the kernel implements is the de-facto standard. If unix(7) differs in anything from what the code does please send patches to the manpages maintainer.
Quoted 4 lines
Oops, sorry, we are not talking about abstract namespace actually (their sockaddr length are necessarily bigger than sizeof(sa_family_t) since they need some data), but unamed sockets. So the Address Format paragraph just misses description of unnamed sockets.
How about this?
--- unix.7.orig 2008-03-24 12:24:37.000000000 +0000
+++ unix.7 2008-03-24 12:24:56.000000000 +0000
@@ -87,6 +87,15 @@
bytes in
.IR sun_path .
Note that names in the abstract namespace are not zero-terminated.
+If the size returned by
+.BR accept
+or
+.BR getpeername
+is
+.IR sizeof(sa_family_t) ,
+then it refers to a unnamed socket and
+.I sun_path
+should not be read.
.SS Socket Options
For historical reasons these socket options are specified with a .B SOL_SOCKET
Samuel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Signature · 1 line

I'll likely only see replies if they are CCed to mtk.manpages at gmail dot com

Samuel Thibault

author

2008-03-31 10:10 UTC #7 lore raw

Quoted 2 lines · Michael Kerrisk, le Mon 31 Mar 2008 06:00:50 +0200, a écrit :

My understanding about abstract namespace sockets (what Samuel calls unnamed sockets)

No, unnamed sockets are not in the abstract namespace sockets. They really have _no_ name.

Samuel

Michael Kerrisk

2008-03-31 18:52 UTC #8 lore raw

Quoted 8 lines · Samuel Thibault

Quoted 2 lines · Michael Kerrisk, le Mon 31 Mar 2008 06:00:50 +0200, a écrit :
My understanding about abstract namespace sockets (what Samuel calls unnamed sockets)
No, unnamed sockets are not in the abstract namespace sockets. They
really have _no_ name.

Ahhh -- sorry -- I didn't read the thread closely enough...

Michael Kerrisk

2008-04-18 15:40 UTC #9 lore raw

In reply to Michael Kerrisk

Samuel,

Quoted 33 lines · Michael Kerrisk

Quoted 32 lines · Samuel Thibault
Quoted 30 lines · Samuel Thibault, le Mon 24 Mar 2008 12:17:19 +0000, a écrit :
Quoted 28 lines · Andi Kleen, le Mon 24 Mar 2008 12:50:10 +0100, a écrit :
Quoted 15 lines · Samuel Thibault
Quoted 8 lines · David Miller, le Sun 23 Mar 2008 21:56:41 -0700, a écrit :
From: Samuel Thibault <[email protected]>
Date: Sat, 8 Mar 2008 02:23:21 +0000
Quoted 5 lines
Accept and getpeername are supposed to return the amount of bytes written in the returned address. However, on unnamed sockets, only sizeof(short) is returned, while a 0 is put in the sun_path member. This patch adds 1 for that additional byte.
Quoted 5 lines
Quoted 1 line
Signed-off-by Samuel Thibault <[email protected]>
This change isn't correct. It's the fact that the length returned is sizeof(short) that tells the caller that the unix socket is unnamed.
Quoted 6 lines
Mmm, where that is documented?
I can't find any details about that in SUS, and man 7 unix says
`If sun_path starts with a null byte ('' '), then it refers to the abstract namespace main- tained by the Unix protocol module.'
[I wrote unix(7) originally]. The abstract name space is a Linux extension and there is no written standard and whatever the kernel implements is the de-facto standard. If unix(7) differs in anything from what the code does please send patches to the manpages maintainer.

Quoted 7 lines

Quoted 7 lines
Quoted 4 lines
Oops, sorry, we are not talking about abstract namespace actually (their sockaddr length are necessarily bigger than sizeof(sa_family_t) since they need some data), but unamed sockets. So the Address Format paragraph just misses description of unnamed sockets.
How about this?

The idea of this patch seems okay. But one minor question below.

Quoted 10 lines

Quoted 10 lines
--- unix.7.orig 2008-03-24 12:24:37.000000000 +0000
+++ unix.7 2008-03-24 12:24:56.000000000 +0000
@@ -87,6 +87,15 @@
bytes in
.IR sun_path .
Note that names in the abstract namespace are not zero-terminated.
+If the size returned by
+.BR accept
+or
+.BR getpeername

or getsockname()

Quoted 2 lines

Quoted 2 lines
+is
+.IR sizeof(sa_family_t) ,

Why did you write sa_family_t here? Dave M already said sizeof(short), which is the same thing, and I see that in net/unix/af_unix.c::unix_getname() there is:

        u = unix_sk(sk);

        unix_state_lock(sk);
                if (!u->addr) {

                sunaddr->sun_family = AF_UNIX;

                sunaddr->sun_path[0] = 0;

                *uaddr_len = sizeof(short);
        } else {

Quoted 6 lines

Quoted 6 lines
+then it refers to a unnamed socket and
+.I sun_path
+should not be read.
.SS Socket Options
For historical reasons these socket options are specified with a .B SOL_SOCKET

Cheers,

Michael

Signature · 4 lines

Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Want to report a man-pages bug?  Look here:
http://www.kernel.org/doc/man-pages/reporting_bugs.html

Samuel Thibault

author

2008-04-24 00:17 UTC #10 lore raw

Quoted 4 lines · Michael Kerrisk, le Fri 18 Apr 2008 18:52:21 +0200, a écrit :

Quoted 2 lines
Quoted 2 lines
+is
+.IR sizeof(sa_family_t) ,
Why did you write sa_family_t here?

Because to me it made more sense.

Quoted 1 line

Dave M already said sizeof(short), which is the same thing,

Ok, but that's exposing implementation.

Quoted 3 lines

and I see that in net/unix/af_unix.c::unix_getname() there is:
                *uaddr_len = sizeof(short);

I'd say that code should be fixed into using sa_family_t.

Samuel

Michael Kerrisk

2008-04-24 08:31 UTC #11 lore raw

Quoted 6 lines

Quoted 4 lines
and I see that in net/unix/af_unix.c::unix_getname() there is:
                *uaddr_len = sizeof(short);
I'd say that code should be fixed into using sa_family_t.

Is there a patch for this?

Samuel Thibault

author

2008-04-26 01:56 UTC #12 lore raw

Quoted 8 lines · Michael Kerrisk, le Thu 24 Apr 2008 10:31:15 +0200, a écrit :

Quoted 6 lines
Quoted 4 lines
and I see that in net/unix/af_unix.c::unix_getname() there is:
                *uaddr_len = sizeof(short);
I'd say that code should be fixed into using sa_family_t.
Is there a patch for this?

Here it is

AF_UNIX: make unix_getname use sizeof(sunaddr->sun_family) instead of sizeof(short).

Signed-off-by Samuel Thibault <[email protected]>

net/unix/af_unix.c renamed from net/unix/af_unix.c.orig +1 -1

@@ -1256,7 +1256,7 @@ static int unix_getname(struct socket *s
 	if (!u->addr) {
 		sunaddr->sun_family = AF_UNIX;
 		sunaddr->sun_path[0] = 0;
-		*uaddr_len = sizeof(short);
+		*uaddr_len = sizeof(sunaddr->sun_family);
 	} else {
 		struct unix_address *addr = u->addr;

David Miller

maintainer

2008-04-27 05:54 UTC #13 lore raw

From: Samuel Thibault <[email protected]>
Date: Sat, 26 Apr 2008 02:44:45 +0100

Quoted 4 lines

AF_UNIX: make unix_getname use sizeof(sunaddr->sun_family) instead of sizeof(short).
Signed-off-by Samuel Thibault <[email protected]>

This is just syntactic masterbation, sa_family_t is typedef'd "unsigned short".

No system on planet earth providing the BSD sockets API uses anything other than uint16_t or unsigned short for this.

Sorry, I'm not applying this.

Michael Kerrisk

2008-05-12 13:10 UTC #14 lore raw

Samuel, et al.

Quoted 8 lines · David Miller

From: Samuel Thibault <[email protected]>
Date: Sat, 26 Apr 2008 02:44:45 +0100
Quoted 4 lines
AF_UNIX: make unix_getname use sizeof(sunaddr->sun_family) instead of sizeof(short).
Signed-off-by Samuel Thibault <[email protected]>

Quoted 7 lines

This is just syntactic masterbation, sa_family_t is typedef'd "unsigned short".
No system on planet earth providing the BSD sockets API uses anything other than uint16_t or unsigned short for this.
Sorry, I'm not applying this.

I finally got round to testing on FreeBSD. Linux and BSD are unfortunately not compatible. For the unnamed sockets, FreeBSD says the socket address length is 16 bytes (whereas Linux says it's 2 bytes). On the other hand, there doesn't seem to be much consistency across implementations: HP-UX's get{peer,sock}name() says that unnamed sockets have a zero-length address.

Anyway, the situation we have is three address formats in the Unix domain on Linux:

Named sockets (socket was given an string name with bind()) length >= 4 (i.e., sizeof(unsigned short) + at least one character for a pathname + 1 for the NUL
sun_path is a null-terminated string

Abstract sockets (socket was bound to a sun_path whose initial byte is 0) Length == sizeof(struct sockaddr_un) (i.e., 110)
sun_path is an initial null byte, followed by 107 other bytes that make the name unique

Unnamed sockets (created by socketpair(), or when we connect() a socket that was not bound to a name; the current unix.7 page suggests that when we connect() a socket that was not bound, then it gets a name in the abstract name space -- that's not true)
Length = 2
sun_path is 108 null bytes .

I have drafted a revision to section of the unix.7 page describing the address format to try and cover all of the above. Samuel (and David?) could you review please?

Cheers,

Michael

Address Format
A Unix domain socket address is represented in the following structure:

#define UNIX_PATH_MAX 108

struct sockaddr_un {
sa_family_t sun_family; /* AF_UNIX */
char sun_path[UNIX_PATH_MAX]; /* pathname */
};

sun_family always contains AF_UNIX.

Three types of address are distinguished in this structure:

* pathname: a Unix domain socket can be bound to a null-ter-
minated file system pathname using bind(2). When the
address of the socket is returned by getsockname(2), get- peername(2), and accept(2), its length is sizeof(sa_fam- ily_t) + strlen(sun_path) + 1, and sun_path contains the null-terminated pathname.

* anonymous: A stream socket that is connect(2)ed to another socket without first being bound to an address is anony- mous. Likewise, the two sockets created by socketpair(2) are anonymous. When the address of an anonymous socket is returned by getsockname(2), getpeername(2), and accept(2), its length is sizeof(sa_family_t), and sun_path should not be inspected.

* abstract: an abstract socket address is distinguished by the fact that sun_path[0] is a null byte (`\0'). All of the remaining bytes in sun_path define the "name" of the
socket. (Null bytes in the name have no special signifi-
cance.) The name has no connection with file system path- names. The socket's address in this namespace is given by the rest of the bytes in sun_path. When the address of an abstract socket is returned by getsockname(2), getpeer- name(2), and accept(2), its length is sizeof(struct sock- addr_un), and sun_path contains the abstract name. The abstract socket namespace is a non-portable Linux exten- sion.

== END ==

Samuel Thibault

author

2008-05-12 14:17 UTC #15 lore raw

Hello,

Quoted 2 lines · Michael Kerrisk, le Mon 12 May 2008 15:10:04 +0200, a écrit :

I have drafted a revision to section of the unix.7 page describing the address format to try and cover all of the above.

Looks much more clear.

Samuel

David Miller

maintainer

2008-03-24 20:24 UTC #16 lore raw

In reply to Samuel Thibault

From: Samuel Thibault <[email protected]>
Date: Mon, 24 Mar 2008 10:43:30 +0000

Quoted 2 lines

See documentation above. If applications don't follow documentation, then they deserve breaking :)

Not when we've been reporting the existing value for more than 10 years.