* [patch] Fix handling of overlength pathname in AF_UNIX sun_path @ 2012-04-17 10:44 Michael Kerrisk [not found] ` <4F8D497F.8060601-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2012-04-18 2:36 ` David Miller 0 siblings, 2 replies; 19+ messages in thread From: Michael Kerrisk @ 2012-04-17 10:44 UTC (permalink / raw) To: netdev Cc: Tetsuo Handa, linux-api-u79uwXL29TY76Z2rM5mHXA, yoshfuji-VfPWfsRibaP+Ru+s062T9g, David Miller, Jan Engelhardt, Willy Tarreau, Alan Cox 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; } ^ permalink raw reply [flat|nested] 19+ messages in thread
[parent not found: <4F8D497F.8060601-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [patch] Fix handling of overlength pathname in AF_UNIX sun_path [not found] ` <4F8D497F.8060601-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> @ 2012-04-17 10:51 ` Willy Tarreau [not found] ` <20120417105107.GA8614-K+wRfnb2/UA@public.gmane.org> 2012-04-18 19:44 ` Alan Cox 0 siblings, 2 replies; 19+ messages in thread From: Willy Tarreau @ 2012-04-17 10:51 UTC (permalink / raw) To: Michael Kerrisk Cc: netdev, Tetsuo Handa, linux-api-u79uwXL29TY76Z2rM5mHXA, yoshfuji-VfPWfsRibaP+Ru+s062T9g, David Miller, Jan Engelhardt, Alan Cox Hi Michael, On Tue, Apr 17, 2012 at 10:44:15PM +1200, Michael Kerrisk wrote: (...) > 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? My personal opinion is that (as you said), the risk of breaking existing apps is already fairly low, but we must not deliberately break existing apps. Eventhough there are currently a log, this is exactly what sysctls are made for. I would personally like to have a default limit to 107 chars + one zero, with a sysctl option to revert to current behaviour if ever it broke an application. In my opinion it's exactly comparable to the risk of breaking apps with mmap_min_addr : very low risk but must be covered by a workaround (sysctl). Just my 2 cents, Willy ^ permalink raw reply [flat|nested] 19+ messages in thread
[parent not found: <20120417105107.GA8614-K+wRfnb2/UA@public.gmane.org>]
* Re: [patch] Fix handling of overlength pathname in AF_UNIX sun_path [not found] ` <20120417105107.GA8614-K+wRfnb2/UA@public.gmane.org> @ 2012-04-17 15:43 ` Carlos O'Donell 0 siblings, 0 replies; 19+ messages in thread From: Carlos O'Donell @ 2012-04-17 15:43 UTC (permalink / raw) To: Willy Tarreau Cc: Michael Kerrisk, netdev, Tetsuo Handa, linux-api-u79uwXL29TY76Z2rM5mHXA, yoshfuji-VfPWfsRibaP+Ru+s062T9g, David Miller, Jan Engelhardt, Alan Cox On Tue, Apr 17, 2012 at 6:51 AM, Willy Tarreau <w@1wt.eu> wrote: > Hi Michael, > > On Tue, Apr 17, 2012 at 10:44:15PM +1200, Michael Kerrisk wrote: > (...) >> 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? > > My personal opinion is that (as you said), the risk of breaking existing > apps is already fairly low, but we must not deliberately break existing > apps. Eventhough there are currently a log, this is exactly what sysctls > are made for. I would personally like to have a default limit to 107 chars > + one zero, with a sysctl option to revert to current behaviour if ever it > broke an application. In my opinion it's exactly comparable to the risk of > breaking apps with mmap_min_addr : very low risk but must be covered by a > workaround (sysctl). To further the opinion that the risk is low: The Open Solaris SUN_LEN macro uses strlen to compute the length. The glibc SUN_LEN macro uses strlen to compute the length. The Mac OS X libc SUN_LEN macro uses strlen to compute the length. The linux man-pages project sets the user expectation that it is a `null-terminated string' (circular argument). I see every expectation from userspace/glibc that it should be 107 chars + '\0'. Cheers, Carlos. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [patch] Fix handling of overlength pathname in AF_UNIX sun_path 2012-04-17 10:51 ` Willy Tarreau [not found] ` <20120417105107.GA8614-K+wRfnb2/UA@public.gmane.org> @ 2012-04-18 19:44 ` Alan Cox 1 sibling, 0 replies; 19+ messages in thread From: Alan Cox @ 2012-04-18 19:44 UTC (permalink / raw) To: Willy Tarreau Cc: Michael Kerrisk, netdev, Tetsuo Handa, linux-api, yoshfuji, David Miller, Jan Engelhardt I think its pointless. Yes there may have been a case years ago to nul terminate but thats now how 4BSD defined the API so that's how the world looks. It helps nobody on the app side because they'll be defensively coding for the existing API for another ten years for enterprise distros anyway. Alan ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [patch] Fix handling of overlength pathname in AF_UNIX sun_path 2012-04-17 10:44 [patch] Fix handling of overlength pathname in AF_UNIX sun_path Michael Kerrisk [not found] ` <4F8D497F.8060601-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> @ 2012-04-18 2:36 ` David Miller [not found] ` <20120417.223614.629911246108750471.davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> 1 sibling, 1 reply; 19+ messages in thread From: David Miller @ 2012-04-18 2:36 UTC (permalink / raw) To: mtk.manpages Cc: netdev, penguin-kernel, linux-api, yoshfuji, jengelh, w, alan From: Michael Kerrisk <mtk.manpages@gmail.com> Date: Tue, 17 Apr 2012 22:44:15 +1200 > 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. The problem with this logic is that it ignores every single Linux system that exists right now. You need to code this logic into your application unless you don't want it to run properly on every Linux system that actually exists. Sorry, we're not making this change. ^ permalink raw reply [flat|nested] 19+ messages in thread
[parent not found: <20120417.223614.629911246108750471.davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>]
* Re: [patch] Fix handling of overlength pathname in AF_UNIX sun_path [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> 0 siblings, 1 reply; 19+ messages in thread From: Carlos O'Donell @ 2012-04-18 4:08 UTC (permalink / raw) To: David Miller Cc: mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w, netdev-u79uwXL29TY76Z2rM5mHXA, penguin-kernel-1yMVhJb1mP/7nzcFbJAaVXf5DAMn2ifp, linux-api-u79uwXL29TY76Z2rM5mHXA, yoshfuji-VfPWfsRibaP+Ru+s062T9g, jengelh-nopoi9nDyk+ELgA04lAiVw, w, alan-qBU/x9rampVanCEyBjwyrvXRex20P6io On Tue, Apr 17, 2012 at 10:36 PM, David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> wrote: > From: Michael Kerrisk <mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> > Date: Tue, 17 Apr 2012 22:44:15 +1200 > >> 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. > > The problem with this logic is that it ignores every single Linux > system that exists right now. > > You need to code this logic into your application unless you don't > want it to run properly on every Linux system that actually exists. > > Sorry, we're not making this change. Dave, I don't clearly understand your position here, and perhaps that's my own ignorance, but could you please clarify, with examples, exactly why the change is not acceptable? I can see several valid arguments against the change, but I don't know which argument your position asserts. One might assert that careless userspace applications exist that pass `sizeof(struct sockaddr_un)' (or worse) as the 3rd argument to bind instead of SUN_LEN(my_sock). The logic in the patch doesn't account for this, and can't really, and would therefore unacceptably break existing applications by trying to assert the location of a \0 where one doesn't exist. The kernel must therefore continue to null-terminate at the specified length, possibly the 109th character, and use strlen to capture the true length of the path. The kernel knows that in the worst case a non-null terminated path might contain some garbage, but that's the users fault, and the logic to prevent this must exist in the application not the kernel. The counter argument to all of this is that it's a QoI issue, and that the kernel shouldn't accept accidentally non-null terminated paths, and should instead return EINVAL for them. Not to mention that it's difficult for userspace to easily catch this error in glibc which would need to inspect the sockaddr, duplicating kernel code. Why is it valid for the user path to have no null terminator? Why not have: diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c index d510353..f9f77a7 100644 --- a/net/unix/af_unix.c +++ b/net/unix/af_unix.c @@ -216,6 +216,9 @@ static int unix_mkname(struct sockaddr_un *sunaddr, int len, unsigned *hashp) */ ((char *)sunaddr)[len] = 0; len = strlen(sunaddr->sun_path)+1+sizeof(short); + /* No null terminator was found in the path. */ + if (len > sizeof(*sunaddr)) + return -EINVAL; return len; } --- Does that make sense? Cheers, Carlos. ^ permalink raw reply related [flat|nested] 19+ messages in thread
[parent not found: <CADZpyix6DZ93f8MQf3Aa1NVV7HCFMAXVAdzRMFBY7xWHHQMPog-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [patch] Fix handling of overlength pathname in AF_UNIX sun_path [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 8:17 ` David Laight 1 sibling, 1 reply; 19+ messages in thread From: David Miller @ 2012-04-18 4:16 UTC (permalink / raw) To: carlos-v2tUB8YBRSi3e3T8WW9gsA Cc: mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w, netdev-u79uwXL29TY76Z2rM5mHXA, penguin-kernel-1yMVhJb1mP/7nzcFbJAaVXf5DAMn2ifp, linux-api-u79uwXL29TY76Z2rM5mHXA, yoshfuji-VfPWfsRibaP+Ru+s062T9g, jengelh-nopoi9nDyk+ELgA04lAiVw, w, alan-qBU/x9rampVanCEyBjwyrvXRex20P6io From: "Carlos O'Donell" <carlos-v2tUB8YBRSi3e3T8WW9gsA@public.gmane.org> Date: Wed, 18 Apr 2012 00:08:47 -0400 > I don't clearly understand your position here, and perhaps that's my > own ignorance, but could you please clarify, with examples, exactly > why the change is not acceptable? My position is that since millions upon millions of Linux systems, in fact every single Linux system, exists right now with the current behavior we are not helping application writers at all by changing behavior now after it's been this way for nearly 20 years. Because if an application writer wants his code to work on systems that actually exist he has to accomodate the non-NULL termination situation if he wants to inspect or print out an AF_UNIX path. Because every system in existence right now allows the non-NULL terminated AF_UNIX paths, therefore it's possible on every system in existence right now. Catch my drift? The very thing the patch claims to help, it doesn't. We install this kernel patch now and then tell application writers that they can just assume all AF_UNIX paths are NULL terminated when they want to print it out, because such code will not actually be guarenteed to work on all deployed Linux machines out there. You cannot just ignore 20 years of precedence and say "oh let's change this in the kernel now, and that way application writers don't have to worry about that lack of NULL termination any more." It simply doesn't work like that. All of this talk about whether applications actually create non-NULL terminated AF_UNIX paths don't even factor into the conversation. So the value proposition for this patch simply does not exist. ^ permalink raw reply [flat|nested] 19+ messages in thread
[parent not found: <20120418.001650.1042781402985153056.davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>]
* Re: [patch] Fix handling of overlength pathname in AF_UNIX sun_path [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 22:50 ` Michael Kerrisk (man-pages) 1 sibling, 1 reply; 19+ messages in thread From: Carlos O'Donell @ 2012-04-18 12:57 UTC (permalink / raw) To: David Miller Cc: mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w, netdev-u79uwXL29TY76Z2rM5mHXA, penguin-kernel-1yMVhJb1mP/7nzcFbJAaVXf5DAMn2ifp, linux-api-u79uwXL29TY76Z2rM5mHXA, yoshfuji-VfPWfsRibaP+Ru+s062T9g, jengelh-nopoi9nDyk+ELgA04lAiVw, w, alan-qBU/x9rampVanCEyBjwyrvXRex20P6io On Wed, Apr 18, 2012 at 12:16 AM, David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> wrote: > From: "Carlos O'Donell" <carlos-v2tUB8YBRSi3e3T8WW9gsA@public.gmane.org> > Date: Wed, 18 Apr 2012 00:08:47 -0400 > >> I don't clearly understand your position here, and perhaps that's my >> own ignorance, but could you please clarify, with examples, exactly >> why the change is not acceptable? > > My position is that since millions upon millions of Linux systems, in > fact every single Linux system, exists right now with the current > behavior we are not helping application writers at all by changing > behavior now after it's been this way for nearly 20 years. > > Because if an application writer wants his code to work on systems > that actually exist he has to accomodate the non-NULL termination > situation if he wants to inspect or print out an AF_UNIX path. > > Because every system in existence right now allows the non-NULL > terminated AF_UNIX paths, therefore it's possible on every system > in existence right now. > > Catch my drift? > > The very thing the patch claims to help, it doesn't. We install this > kernel patch now and then tell application writers that they can just > assume all AF_UNIX paths are NULL terminated when they want to print > it out, because such code will not actually be guarenteed to work on > all deployed Linux machines out there. > > You cannot just ignore 20 years of precedence and say "oh let's change > this in the kernel now, and that way application writers don't have to > worry about that lack of NULL termination any more." It simply > doesn't work like that. > > All of this talk about whether applications actually create non-NULL > terminated AF_UNIX paths don't even factor into the conversation. > > So the value proposition for this patch simply does not exist. Thank you, this is the kind of position statement I can point to if I ever get asked about this again. In summary your opinion is that the API has and always will allow up to 108 chars to be used in sun_path? In which case I will talk to the Austin group to get a good example added to POSIX showing safe usage. Cheers, Carlos. ^ permalink raw reply [flat|nested] 19+ messages in thread
[parent not found: <CADZpyixnQMM5WFWLyvEQ=D-tvrqFGe4PC5WdUzxVtdL96NODJQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [patch] Fix handling of overlength pathname in AF_UNIX sun_path [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> 0 siblings, 1 reply; 19+ messages in thread From: David Miller @ 2012-04-18 17:31 UTC (permalink / raw) To: carlos-v2tUB8YBRSi3e3T8WW9gsA Cc: mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w, netdev-u79uwXL29TY76Z2rM5mHXA, penguin-kernel-1yMVhJb1mP/7nzcFbJAaVXf5DAMn2ifp, linux-api-u79uwXL29TY76Z2rM5mHXA, yoshfuji-VfPWfsRibaP+Ru+s062T9g, jengelh-nopoi9nDyk+ELgA04lAiVw, w, alan-qBU/x9rampVanCEyBjwyrvXRex20P6io From: "Carlos O'Donell" <carlos-v2tUB8YBRSi3e3T8WW9gsA@public.gmane.org> Date: Wed, 18 Apr 2012 08:57:58 -0400 > In summary your opinion is that the API has and always will allow up > to 108 chars to be used in sun_path? Yes. > In which case I will talk to the Austin group to get a good example > added to POSIX showing safe usage. Why would you add language to POSIX for Linux specific behavior? Just curious :-) ^ permalink raw reply [flat|nested] 19+ messages in thread
[parent not found: <20120418.133102.1711079292327461659.davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>]
* Re: [patch] Fix handling of overlength pathname in AF_UNIX sun_path [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 0 siblings, 1 reply; 19+ messages in thread From: Carlos O'Donell @ 2012-04-18 18:48 UTC (permalink / raw) To: David Miller Cc: mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w, netdev-u79uwXL29TY76Z2rM5mHXA, penguin-kernel-1yMVhJb1mP/7nzcFbJAaVXf5DAMn2ifp, linux-api-u79uwXL29TY76Z2rM5mHXA, yoshfuji-VfPWfsRibaP+Ru+s062T9g, jengelh-nopoi9nDyk+ELgA04lAiVw, w, alan-qBU/x9rampVanCEyBjwyrvXRex20P6io On Wed, Apr 18, 2012 at 1:31 PM, David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> wrote: > From: "Carlos O'Donell" <carlos-v2tUB8YBRSi3e3T8WW9gsA@public.gmane.org> > Date: Wed, 18 Apr 2012 08:57:58 -0400 > >> In summary your opinion is that the API has and always will allow up >> to 108 chars to be used in sun_path? > > Yes. > >> In which case I will talk to the Austin group to get a good example >> added to POSIX showing safe usage. > > Why would you add language to POSIX for Linux specific behavior? > Just curious :-) Why not? Do you ever feel crazy when people question what you think is perfectly reasonable? ;-) POSIX doesn't exist in a vacuum, we need to harmonize reality with the standard. If an implementation exists where sun_path has no null-terminator then it is useful to have POSIX clarify that null-termination is implementation defined behaviour, just like it says that sun_path's length undefined. Under "Application Usage" or "Examples" it's valid to talk about specific implementations. See: http://pubs.opengroup.org/onlinepubs/007904975/basedefs/sys/un.h.html, where it talks about BSD in the "Application Usage." It's about time we some "Linux this" and "Linux that" in there. Cheers, Carlos. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [patch] Fix handling of overlength pathname in AF_UNIX sun_path 2012-04-18 18:48 ` Carlos O'Donell @ 2012-04-18 19:23 ` David Miller 0 siblings, 0 replies; 19+ messages in thread From: David Miller @ 2012-04-18 19:23 UTC (permalink / raw) To: carlos Cc: mtk.manpages, netdev, penguin-kernel, linux-api, yoshfuji, jengelh, w, alan From: "Carlos O'Donell" <carlos@systemhalted.org> Date: Wed, 18 Apr 2012 14:48:43 -0400 > POSIX doesn't exist in a vacuum, we need to harmonize reality with the > standard. If an implementation exists where sun_path has no > null-terminator then it is useful to have POSIX clarify that > null-termination is implementation defined behaviour, just like it > says that sun_path's length undefined. Under "Application Usage" or > "Examples" it's valid to talk about specific implementations. > > See: http://pubs.opengroup.org/onlinepubs/007904975/basedefs/sys/un.h.html, > where it talks about BSD in the "Application Usage." It's about time > we some "Linux this" and "Linux that" in there. Ok, thanks for explaining. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [patch] Fix handling of overlength pathname in AF_UNIX sun_path [not found] ` <20120418.001650.1042781402985153056.davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> 2012-04-18 12:57 ` Carlos O'Donell @ 2012-04-18 22:50 ` Michael Kerrisk (man-pages) [not found] ` <CAKgNAkh46EMDWpessyi0n-EyNoRid-iW1O1RfUpTtzKDv0mZFw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 1 sibling, 1 reply; 19+ messages in thread From: Michael Kerrisk (man-pages) @ 2012-04-18 22:50 UTC (permalink / raw) To: David Miller Cc: carlos-v2tUB8YBRSi3e3T8WW9gsA, netdev-u79uwXL29TY76Z2rM5mHXA, penguin-kernel-1yMVhJb1mP/7nzcFbJAaVXf5DAMn2ifp, linux-api-u79uwXL29TY76Z2rM5mHXA, yoshfuji-VfPWfsRibaP+Ru+s062T9g, jengelh-nopoi9nDyk+ELgA04lAiVw, w, alan-qBU/x9rampVanCEyBjwyrvXRex20P6io On Wed, Apr 18, 2012 at 4:16 PM, David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> wrote: > From: "Carlos O'Donell" <carlos-v2tUB8YBRSi3e3T8WW9gsA@public.gmane.org> > Date: Wed, 18 Apr 2012 00:08:47 -0400 > >> I don't clearly understand your position here, and perhaps that's my >> own ignorance, but could you please clarify, with examples, exactly >> why the change is not acceptable? > > My position is that since millions upon millions of Linux systems, in > fact every single Linux system, exists right now with the current > behavior we are not helping application writers at all by changing > behavior now after it's been this way for nearly 20 years. > > Because if an application writer wants his code to work on systems > that actually exist he has to accomodate the non-NULL termination > situation if he wants to inspect or print out an AF_UNIX path. > > Because every system in existence right now allows the non-NULL > terminated AF_UNIX paths, therefore it's possible on every system > in existence right now. > > Catch my drift? > > The very thing the patch claims to help, it doesn't. We install this > kernel patch now and then tell application writers that they can just > assume all AF_UNIX paths are NULL terminated when they want to print > it out, because such code will not actually be guarenteed to work on > all deployed Linux machines out there. Hang on a moment. I did not suggest that we can just tell users they can forget about the past. Obviously, users will need to program to past kernel behavior here for a good long time yet. (As Alan says elsewhere in the thread "they'll be defensively coding for the existing API for another ten years for enterprise distros anyway".) However, this is about longer-term improvement of the quality of implementation; in X years (choose your X) time, a lot of new application may not need to care about the old broken behavior. See some related examples below. And you skipped past my other two points. Even if my understanding about POSIX mandates is correct, I can understand how we might ignore that point. But the last one is still germane: [[ 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. ]] There may well be potential breakages out there in set (a), and improving the QOI would help them. (To put things in terms of Alan's response: I suspect that there may well be existing applications that are *not* defensively handling the existing API). Taking the logic you've posed (my reading: "we shouldn't fix old brokenness because applications will still need to code to the brokenness") to the extreme, we'd *never* fix old pieces of brokenness. However, we certainly have precedents for doing exactly that: After nearly 15 years of brokenness (stretching back to the first kernels), commit 69be8f189653cd81aae5a74e26615b12871bb72e fixed this (sigaction(2)): BUGS In kernels up to and including 2.6.13, specifying SA_NODEFER in sa_flags prevents not only the delivered signal from being masked during execution of the handler, but also the signals specified in sa_mask. This bug was fixed in kernel 2.6.14. Similarly, after brokenness that had run through the entire preceding 2.4.x kernel series, Linux 2.6.4 fixed this: BUGS In kernel 2.4 (and earlier) there is some strangeness in the handling of X_OK tests for superuser. If all categories of execute permission are disabled for a nondirectory file, then the only access() test that returns -1 is when mode is speci‐ fied as just X_OK; if R_OK or W_OK is also specified in mode, then access() returns 0 for such files. Early 2.6 kernels (up to and including 2.6.3) also behaved in the same way as kernel 2.4. (A little background here: http://thread.gmane.org/gmane.linux.kernel/158814, and the fix eventually went in with http://thread.gmane.org/gmane.linux.kernel/178719) > You cannot just ignore 20 years of precedence and say "oh let's change > this in the kernel now, and that way application writers don't have to > worry about that lack of NULL termination any more." It simply > doesn't work like that. As should be clear from the above, I agree. But still, I don't think the logic "it's broken, and even if we fix it, users will still have to code to the old brokenness" is a sufficient argument against improving the QOI long-term. > All of this talk about whether applications actually create non-NULL > terminated AF_UNIX paths don't even factor into the conversation. > > So the value proposition for this patch simply does not exist. Of course, it's your call in the end, but I don't think things are as cut-and-dried as your response suggests. Cheers, Michael -- Michael Kerrisk Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/ Author of "The Linux Programming Interface"; http://man7.org/tlpi/ ^ permalink raw reply [flat|nested] 19+ messages in thread
[parent not found: <CAKgNAkh46EMDWpessyi0n-EyNoRid-iW1O1RfUpTtzKDv0mZFw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [patch] Fix handling of overlength pathname in AF_UNIX sun_path [not found] ` <CAKgNAkh46EMDWpessyi0n-EyNoRid-iW1O1RfUpTtzKDv0mZFw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2012-04-18 23:31 ` David Miller 2012-04-19 10:19 ` Alan Cox 1 sibling, 0 replies; 19+ messages in thread From: David Miller @ 2012-04-18 23:31 UTC (permalink / raw) To: mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w Cc: carlos-v2tUB8YBRSi3e3T8WW9gsA, netdev-u79uwXL29TY76Z2rM5mHXA, penguin-kernel-1yMVhJb1mP/7nzcFbJAaVXf5DAMn2ifp, linux-api-u79uwXL29TY76Z2rM5mHXA, yoshfuji-VfPWfsRibaP+Ru+s062T9g, jengelh-nopoi9nDyk+ELgA04lAiVw, w, alan-qBU/x9rampVanCEyBjwyrvXRex20P6io From: "Michael Kerrisk (man-pages)" <mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> Date: Thu, 19 Apr 2012 10:50:40 +1200 > However, this is about longer-term improvement of the quality of > implementation; in X years (choose your X) time, a lot of new > application may not need to care about the old broken behavior. There is really no value to this, the AF_UNIX NULL termination issue is significantly different from the signal examples you mention. If we're going to, like Carlos will, make mention in POSIX documents that one must account for possible lack of NULL termination, there is absolutely ZERO value in changing things because we are telling application writers the state of reality which is that they have to allot for this. Please drop this issue, the discussion was over a long time ago, thank you very much. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [patch] Fix handling of overlength pathname in AF_UNIX sun_path [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> 1 sibling, 1 reply; 19+ messages in thread From: Alan Cox @ 2012-04-19 10:19 UTC (permalink / raw) To: mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w Cc: David Miller, carlos-v2tUB8YBRSi3e3T8WW9gsA, netdev-u79uwXL29TY76Z2rM5mHXA, penguin-kernel-1yMVhJb1mP/7nzcFbJAaVXf5DAMn2ifp, linux-api-u79uwXL29TY76Z2rM5mHXA, yoshfuji-VfPWfsRibaP+Ru+s062T9g, jengelh-nopoi9nDyk+ELgA04lAiVw, w > 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. Or another way of putting it 3(a) Sloppy coding that may have lots of other bugs 3(b) Interfaces and code we promised not to break. Alan ^ permalink raw reply [flat|nested] 19+ messages in thread
[parent not found: <20120419111909.616bef70-38n7/U1jhRXW96NNrWNlrekiAK3p4hvP@public.gmane.org>]
* Re: [patch] Fix handling of overlength pathname in AF_UNIX sun_path [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> 0 siblings, 1 reply; 19+ messages in thread From: Michael Kerrisk (man-pages) @ 2012-04-19 10:33 UTC (permalink / raw) To: Alan Cox Cc: David Miller, carlos-v2tUB8YBRSi3e3T8WW9gsA, netdev-u79uwXL29TY76Z2rM5mHXA, penguin-kernel-1yMVhJb1mP/7nzcFbJAaVXf5DAMn2ifp, linux-api-u79uwXL29TY76Z2rM5mHXA, yoshfuji-VfPWfsRibaP+Ru+s062T9g, jengelh-nopoi9nDyk+ELgA04lAiVw, w On Thu, Apr 19, 2012 at 10:19 PM, Alan Cox <alan-qBU/x9rampVanCEyBjwyrvXRex20P6io@public.gmane.org> wrote: >> 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. > > Or another way of putting it > > 3(a) Sloppy coding that may have lots of other bugs > > 3(b) Interfaces and code we promised not to break. Yes, it's another way of putting it. (Though regarding 3(b), part of the problem is that there never was a clearly specified contract.) Anyway, I've dug deeper, looking at hat happens on other platforms. It's a mess: the BSDs don't even guarantee that sun_path is null_terminated. So, here's how one has to portably deal with the variations: addrlen = sizeof(struct sockaddr_un); cfd = accept(lfd, &addr, &addlen); printf("%.*s", addrlen - offsetof(struct sockaddr_un, sun_path), addr.sun_path); That's pretty hideous! Thanks, Michael -- Michael Kerrisk Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/ Author of "The Linux Programming Interface"; http://man7.org/tlpi/ ^ permalink raw reply [flat|nested] 19+ messages in thread
[parent not found: <CAKgNAkjZ31JAs0XFutnAozCAcHHAq6pcCAKeDNHccKQ+6uTP6g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [patch] Fix handling of overlength pathname in AF_UNIX sun_path [not found] ` <CAKgNAkjZ31JAs0XFutnAozCAcHHAq6pcCAKeDNHccKQ+6uTP6g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2012-04-19 12:11 ` Jan Engelhardt 0 siblings, 0 replies; 19+ messages in thread From: Jan Engelhardt @ 2012-04-19 12:11 UTC (permalink / raw) To: Michael Kerrisk (man-pages) Cc: Alan Cox, David Miller, carlos-v2tUB8YBRSi3e3T8WW9gsA, netdev-u79uwXL29TY76Z2rM5mHXA, penguin-kernel-1yMVhJb1mP/7nzcFbJAaVXf5DAMn2ifp, linux-api-u79uwXL29TY76Z2rM5mHXA, yoshfuji-VfPWfsRibaP+Ru+s062T9g, w On Thursday 2012-04-19 12:33, Michael Kerrisk (man-pages) wrote: > >Anyway, I've dug deeper, looking at hat happens on other platforms. >It's a mess: the BSDs don't even guarantee that sun_path is >null_terminated. So, here's how one has to portably deal with the >variations: > >addrlen = sizeof(struct sockaddr_un); >cfd = accept(lfd, &addr, &addlen); > >printf("%.*s", addrlen - offsetof(struct sockaddr_un, sun_path), addr.sun_path); What operating system made you write that? I just ask for fun and the record. Solaris's dirent is also constructed such that using sizeof(...) is useless. typedef struct dirent { ino_t d_ino; /* "inode number" of entry */ off_t d_off; /* offset of disk directory entry */ unsigned short d_reclen; /* length of this record */ char d_name[1]; /* name of file */ } dirent_t; ^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [patch] Fix handling of overlength pathname in AF_UNIX sun_path [not found] ` <CADZpyix6DZ93f8MQf3Aa1NVV7HCFMAXVAdzRMFBY7xWHHQMPog-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2012-04-18 8:17 ` David Laight 2012-04-18 8:17 ` David Laight 1 sibling, 0 replies; 19+ messages in thread From: David Laight @ 2012-04-18 8:17 UTC (permalink / raw) To: Carlos O'Donell, David Miller Cc: mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w, netdev-u79uwXL29TY76Z2rM5mHXA, penguin-kernel-1yMVhJb1mP/7nzcFbJAaVXf5DAMn2ifp, linux-api-u79uwXL29TY76Z2rM5mHXA, yoshfuji-VfPWfsRibaP+Ru+s062T9g, jengelh-nopoi9nDyk+ELgA04lAiVw, w, alan-qBU/x9rampVanCEyBjwyrvXRex20P6io > > Why not have: > > diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c > index d510353..f9f77a7 100644 > --- a/net/unix/af_unix.c > +++ b/net/unix/af_unix.c > @@ -216,6 +216,9 @@ static int unix_mkname(struct sockaddr_un > *sunaddr, int len, unsigned *hashp) > */ > ((char *)sunaddr)[len] = 0; > len = strlen(sunaddr->sun_path)+1+sizeof(short); > + /* No null terminator was found in the path. */ > + if (len > sizeof(*sunaddr)) > + return -EINVAL; > return len; That could generate a kernel page fault! (Depending on what follows (or rather doesn't follow!) sun_path.) You'd need to use memchr() not strlen(). David ^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [patch] Fix handling of overlength pathname in AF_UNIX sun_path @ 2012-04-18 8:17 ` David Laight 0 siblings, 0 replies; 19+ messages in thread From: David Laight @ 2012-04-18 8:17 UTC (permalink / raw) To: Carlos O'Donell, David Miller Cc: mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w, netdev-u79uwXL29TY76Z2rM5mHXA, penguin-kernel-1yMVhJb1mP/7nzcFbJAaVXf5DAMn2ifp, linux-api-u79uwXL29TY76Z2rM5mHXA, yoshfuji-VfPWfsRibaP+Ru+s062T9g, jengelh-nopoi9nDyk+ELgA04lAiVw, w, alan-qBU/x9rampVanCEyBjwyrvXRex20P6io > > Why not have: > > diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c > index d510353..f9f77a7 100644 > --- a/net/unix/af_unix.c > +++ b/net/unix/af_unix.c > @@ -216,6 +216,9 @@ static int unix_mkname(struct sockaddr_un > *sunaddr, int len, unsigned *hashp) > */ > ((char *)sunaddr)[len] = 0; > len = strlen(sunaddr->sun_path)+1+sizeof(short); > + /* No null terminator was found in the path. */ > + if (len > sizeof(*sunaddr)) > + return -EINVAL; > return len; That could generate a kernel page fault! (Depending on what follows (or rather doesn't follow!) sun_path.) You'd need to use memchr() not strlen(). David ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [patch] Fix handling of overlength pathname in AF_UNIX sun_path 2012-04-18 8:17 ` David Laight (?) @ 2012-04-18 13:13 ` Thadeu Lima de Souza Cascardo -1 siblings, 0 replies; 19+ messages in thread From: Thadeu Lima de Souza Cascardo @ 2012-04-18 13:13 UTC (permalink / raw) To: David Laight Cc: Carlos O'Donell, David Miller, mtk.manpages, netdev, penguin-kernel, linux-api, yoshfuji, jengelh, w, alan On Wed, Apr 18, 2012 at 09:17:26AM +0100, David Laight wrote: > > > > > Why not have: > > > > diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c > > index d510353..f9f77a7 100644 > > --- a/net/unix/af_unix.c > > +++ b/net/unix/af_unix.c > > @@ -216,6 +216,9 @@ static int unix_mkname(struct sockaddr_un > > *sunaddr, int len, unsigned *hashp) > > */ > > ((char *)sunaddr)[len] = 0; > > len = strlen(sunaddr->sun_path)+1+sizeof(short); > > + /* No null terminator was found in the path. */ > > + if (len > sizeof(*sunaddr)) > > + return -EINVAL; > > return len; > > That could generate a kernel page fault! > (Depending on what follows (or rather doesn't follow!) sun_path.) > You'd need to use memchr() not strlen(). > > David > Hi, David. What follows is a 0 byte, because it's set that way in the line before strlen. Note that len is tested for sizeof(*sunaddr), and there is a huge comment about that extra byte that was omitted. The whole function is at net/unix/af_unix.c:203. Regards, Cascardo. ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2012-04-19 12:11 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-04-17 10:44 [patch] Fix handling of overlength pathname in AF_UNIX sun_path Michael Kerrisk [not found] ` <4F8D497F.8060601-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2012-04-17 10:51 ` 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
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.