From: Michael Kerrisk <mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
To: netdev <netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Cc: Tetsuo Handa
<penguin-kernel-1yMVhJb1mP/7nzcFbJAaVXf5DAMn2ifp@public.gmane.org>,
linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
yoshfuji-VfPWfsRibaP+Ru+s062T9g@public.gmane.org,
David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>,
Jan Engelhardt <jengelh-nopoi9nDyk+ELgA04lAiVw@public.gmane.org>,
Willy Tarreau <w@1wt.eu>,
Alan Cox <alan-qBU/x9rampVanCEyBjwyrvXRex20P6io@public.gmane.org>
Subject: [patch] Fix handling of overlength pathname in AF_UNIX sun_path
Date: Tue, 17 Apr 2012 22:44:15 +1200 [thread overview]
Message-ID: <4F8D497F.8060601@gmail.com> (raw)
Tetsuo Handa sent me a patch to document the kernel's odd
behavior when asked to create a UNIX domain socket address where the
pathname completely fills the sun_path field without including a null
terminator [1].
One of the consequences of the current kernel behavior is that
when a socket address is returned to userspace (via getsockname(),
getpeername(), accept(), recvfrom()), applications can't reliably
do things such as:
printf("%s\n", addr.sun_path);
Instead one must either write:
printf("%.*s\n", sizeof(addr.sun_path), addr.sun_path);
or, when retrieving a socket address structure, employ a buffer whose
size is:
sizeof(struct sockaddr_un) + 1
(This ensures that there is enough space to hold the null terminator
for the case where a socket was bound to a sun_path with non-NUL characters
in all 108 bytes. But it entails some casting.)
Tetsuo initially considered there might be a kernel bug here,
but an attempt to change the kernel behavior met resistance [2].
The patch at the end of this message is a slightly different
fix for the same problem. There are a few reasons why I think
it (or some variation) should be considered:
1. Changing the kernel behavior prevents userspace having
to go through the sort of contortions described above
in order to handle the 108-non-NUL-bytes-in-sun_path case.
2. POSIX says that sun_path is a pathname. By (POSIX) definition,
a pathname is null terminated.
3. Considering these two sets:
(a) [applications that rely on the assumption that there
is a null terminator inside sizeof(sun_path) bytes]
(b) [applications that would break if the kernel behavior changed]
I suspect that set (a) is rather larger than set (b)--or, more
likely still, applications ensure they go for the lowest common
denominator limit of 92 (HP-UX) or 104 (historical BSD limit)
bytes, and so avoid this issue completely.
The accompanying patch changes unix_mkname() to ensure that a terminating
null byte is always located within the first 108 bytes of sun_path.
It does change the ABI for the former case where a pathname ran to 108
bytes without a null terminator: for that case, the call now fails with
the error -EINVAL. What are people's thoughts on applying this?
[1] http://thread.gmane.org/gmane.linux.network/174473/focus=1861
[2] http://thread.gmane.org/gmane.linux.kernel/291038
Signed-off-by: Michael Kerrisk <mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
--- net/unix/af_unix.c.orig 2012-04-17 09:50:07.383459311 +1200
+++ net/unix/af_unix.c 2012-04-17 19:49:56.077852639 +1200
@@ -207,14 +207,16 @@ static int unix_mkname(struct sockaddr_u
if (!sunaddr || sunaddr->sun_family != AF_UNIX)
return -EINVAL;
if (sunaddr->sun_path[0]) {
- /*
- * This may look like an off by one error but it is a bit more
- * subtle. 108 is the longest valid AF_UNIX path for a binding.
- * sun_path[108] doesn't as such exist. However in kernel space
- * we are guaranteed that it is a valid memory location in our
- * kernel address buffer.
- */
- ((char *)sunaddr)[len] = 0;
+ if (len == sizeof(*sunaddr)) {
+ /*
+ * If 'sun_path' is completely filled, the user
+ * must include a terminator
+ */
+ if (!memchr(sunaddr->sun_path, '\0',
+ sizeof(sunaddr->sun_path)))
+ return -EINVAL;
+ } else
+ ((char *)sunaddr)[len] = 0;
len = strlen(sunaddr->sun_path)+1+sizeof(short);
return len;
}
next reply other threads:[~2012-04-17 10:44 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-04-17 10:44 Michael Kerrisk [this message]
[not found] ` <4F8D497F.8060601-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2012-04-17 10:51 ` [patch] Fix handling of overlength pathname in AF_UNIX sun_path Willy Tarreau
[not found] ` <20120417105107.GA8614-K+wRfnb2/UA@public.gmane.org>
2012-04-17 15:43 ` Carlos O'Donell
2012-04-18 19:44 ` Alan Cox
2012-04-18 2:36 ` David Miller
[not found] ` <20120417.223614.629911246108750471.davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
2012-04-18 4:08 ` Carlos O'Donell
[not found] ` <CADZpyix6DZ93f8MQf3Aa1NVV7HCFMAXVAdzRMFBY7xWHHQMPog-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2012-04-18 4:16 ` David Miller
[not found] ` <20120418.001650.1042781402985153056.davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
2012-04-18 12:57 ` Carlos O'Donell
[not found] ` <CADZpyixnQMM5WFWLyvEQ=D-tvrqFGe4PC5WdUzxVtdL96NODJQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2012-04-18 17:31 ` David Miller
[not found] ` <20120418.133102.1711079292327461659.davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
2012-04-18 18:48 ` Carlos O'Donell
2012-04-18 19:23 ` David Miller
2012-04-18 22:50 ` Michael Kerrisk (man-pages)
[not found] ` <CAKgNAkh46EMDWpessyi0n-EyNoRid-iW1O1RfUpTtzKDv0mZFw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2012-04-18 23:31 ` David Miller
2012-04-19 10:19 ` Alan Cox
[not found] ` <20120419111909.616bef70-38n7/U1jhRXW96NNrWNlrekiAK3p4hvP@public.gmane.org>
2012-04-19 10:33 ` Michael Kerrisk (man-pages)
[not found] ` <CAKgNAkjZ31JAs0XFutnAozCAcHHAq6pcCAKeDNHccKQ+6uTP6g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2012-04-19 12:11 ` Jan Engelhardt
2012-04-18 8:17 ` David Laight
2012-04-18 8:17 ` David Laight
2012-04-18 13:13 ` Thadeu Lima de Souza Cascardo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4F8D497F.8060601@gmail.com \
--to=mtk.manpages-re5jqeeqqe8avxtiumwx3w@public.gmane.org \
--cc=alan-qBU/x9rampVanCEyBjwyrvXRex20P6io@public.gmane.org \
--cc=davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org \
--cc=jengelh-nopoi9nDyk+ELgA04lAiVw@public.gmane.org \
--cc=linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=penguin-kernel-1yMVhJb1mP/7nzcFbJAaVXf5DAMn2ifp@public.gmane.org \
--cc=w@1wt.eu \
--cc=yoshfuji-VfPWfsRibaP+Ru+s062T9g@public.gmane.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.