Message ID | 20140428160420.GA23142@domone.podge |
---|---|
State | New |
Headers | show |
From: Ondřej Bílka <neleai@seznam.cz> Date: Mon, 28 Apr 2014 18:04:20 +0200 > * sysdeps/unix/sysv/linux/recvmmsg.c (recvmmsg): Do not clobber > timeout argument. It is extremely unfortunate if we've defined this argument as const, now you are making it so that the user has no mechanism to get the updated timeval other than to define their own syscall stubs. This is doubly unfortunately since there is absolutely no reason for us to have defined the interface different from what the kernel actually provides.
On Mon, Apr 28, 2014 at 06:04:20PM +0200, Ondřej Bílka wrote: > diff --git a/sysdeps/unix/sysv/linux/recvmmsg.c b/sysdeps/unix/sysv/linux/recvmmsg.c > index 57ddf31..04a065f 100644 > --- a/sysdeps/unix/sysv/linux/recvmmsg.c > +++ b/sysdeps/unix/sysv/linux/recvmmsg.c > @@ -35,14 +35,16 @@ > #ifdef __NR_recvmmsg > int > recvmmsg (int fd, struct mmsghdr *vmessages, unsigned int vlen, int flags, > - const struct timespec *tmo) > + const struct timespec *_tmo) > { > + struct timespec tmo = *_tmo; > + This is invalid because a null tmo argument is allowed (and is in fact the normal usage case). You have to check for it. Rich
On 04/28/2014 07:01 PM, David Miller wrote: > From: Ondřej Bílka <neleai@seznam.cz> > Date: Mon, 28 Apr 2014 18:04:20 +0200 > >> * sysdeps/unix/sysv/linux/recvmmsg.c (recvmmsg): Do not clobber >> timeout argument. > > It is extremely unfortunate if we've defined this argument as const, > now you are making it so that the user has no mechanism to get the > updated timeval other than to define their own syscall stubs. > > This is doubly unfortunately since there is absolutely no reason for > us to have defined the interface different from what the kernel > actually provides. Agreed. Surely, the right fix here is simply to remove the "const" qualifier from the glibc API? Cheers, Michael
On Wed, Apr 30, 2014 at 10:28:53AM +0200, Michael Kerrisk (man-pages) wrote: > On 04/28/2014 07:01 PM, David Miller wrote: > > From: Ondřej Bílka <neleai@seznam.cz> > > Date: Mon, 28 Apr 2014 18:04:20 +0200 > > > >> * sysdeps/unix/sysv/linux/recvmmsg.c (recvmmsg): Do not clobber > >> timeout argument. > > > > It is extremely unfortunate if we've defined this argument as const, > > now you are making it so that the user has no mechanism to get the > > updated timeval other than to define their own syscall stubs. > > > > This is doubly unfortunately since there is absolutely no reason for > > us to have defined the interface different from what the kernel > > actually provides. > > Agreed. Surely, the right fix here is simply to remove the "const" > qualifier from the glibc API? > Now I am also for removing const. Initially I looked to documentation and this was undocumented feature so I was not sure about it. Could we also document this in manpages?
Hello Ondřej, On 04/30/2014 11:07 AM, Ondřej Bílka wrote: > On Wed, Apr 30, 2014 at 10:28:53AM +0200, Michael Kerrisk (man-pages) wrote: >> On 04/28/2014 07:01 PM, David Miller wrote: >>> From: Ondřej Bílka <neleai@seznam.cz> >>> Date: Mon, 28 Apr 2014 18:04:20 +0200 >>> >>>> * sysdeps/unix/sysv/linux/recvmmsg.c (recvmmsg): Do not clobber >>>> timeout argument. >>> >>> It is extremely unfortunate if we've defined this argument as const, >>> now you are making it so that the user has no mechanism to get the >>> updated timeval other than to define their own syscall stubs. >>> >>> This is doubly unfortunately since there is absolutely no reason for >>> us to have defined the interface different from what the kernel >>> actually provides. >> >> Agreed. Surely, the right fix here is simply to remove the "const" >> qualifier from the glibc API? >> > Now I am also for removing const. Initially I looked to documentation > and this was undocumented feature so I was not sure about it. Yes, my mistake. > Could we > also document this in manpages? Yes, sure, I'll do that. Do you think we also need to document the glibc 'const' issue under a BUGS section? Cheers, Michael
On Wed, Apr 30, 2014 at 1:17 PM, Michael Kerrisk (man-pages) <mtk.manpages@gmail.com> wrote: > Hello Ondřej, > > On 04/30/2014 11:07 AM, Ondřej Bílka wrote: >> On Wed, Apr 30, 2014 at 10:28:53AM +0200, Michael Kerrisk (man-pages) wrote: >>> On 04/28/2014 07:01 PM, David Miller wrote: >>>> From: Ondřej Bílka <neleai@seznam.cz> >>>> Date: Mon, 28 Apr 2014 18:04:20 +0200 >>>> >>>>> * sysdeps/unix/sysv/linux/recvmmsg.c (recvmmsg): Do not clobber >>>>> timeout argument. >>>> >>>> It is extremely unfortunate if we've defined this argument as const, >>>> now you are making it so that the user has no mechanism to get the >>>> updated timeval other than to define their own syscall stubs. >>>> >>>> This is doubly unfortunately since there is absolutely no reason for >>>> us to have defined the interface different from what the kernel >>>> actually provides. >>> >>> Agreed. Surely, the right fix here is simply to remove the "const" >>> qualifier from the glibc API? >>> >> Now I am also for removing const. Initially I looked to documentation >> and this was undocumented feature so I was not sure about it. > > Yes, my mistake. Sigh! Now I remember why that timeout behavior didn't get documented: http://thread.gmane.org/gmane.linux.man/3477/focus=3500 The timeout argument is completely broken, AFAICT. I never did get a reply from Arnaldo (but, as you can see in the thread, others agreed tha there's a problem), and then I got distracted :-{. Cheers, Michael
On Wed, Apr 30, 2014 at 03:35:40PM +0200, Michael Kerrisk (man-pages) wrote: > On Wed, Apr 30, 2014 at 1:17 PM, Michael Kerrisk (man-pages) > <mtk.manpages@gmail.com> wrote: > > Hello Ondřej, > > > > On 04/30/2014 11:07 AM, Ondřej Bílka wrote: > >> On Wed, Apr 30, 2014 at 10:28:53AM +0200, Michael Kerrisk (man-pages) wrote: > >>> On 04/28/2014 07:01 PM, David Miller wrote: > >>>> From: Ondřej Bílka <neleai@seznam.cz> > >>>> Date: Mon, 28 Apr 2014 18:04:20 +0200 > >>>> > >>>>> * sysdeps/unix/sysv/linux/recvmmsg.c (recvmmsg): Do not clobber > >>>>> timeout argument. > >>>> > >>>> It is extremely unfortunate if we've defined this argument as const, > >>>> now you are making it so that the user has no mechanism to get the > >>>> updated timeval other than to define their own syscall stubs. > >>>> > >>>> This is doubly unfortunately since there is absolutely no reason for > >>>> us to have defined the interface different from what the kernel > >>>> actually provides. > >>> > >>> Agreed. Surely, the right fix here is simply to remove the "const" > >>> qualifier from the glibc API? > >>> > >> Now I am also for removing const. Initially I looked to documentation > >> and this was undocumented feature so I was not sure about it. > > > > Yes, my mistake. > > Sigh! Now I remember why that timeout behavior didn't get documented: > > http://thread.gmane.org/gmane.linux.man/3477/focus=3500 > > The timeout argument is completely broken, AFAICT. I never did get a > reply from Arnaldo (but, as you can see in the thread, others agreed > tha there's a problem), and then I got distracted :-{. Indeed, this looks like a big problem and I don't see any viable solution. I think the documented behavior, for now, should be that the behavior is unspecified unless the timeout is a null pointer. Rich
On Wed, Apr 30, 2014 at 6:04 PM, Rich Felker <dalias@libc.org> wrote: > On Wed, Apr 30, 2014 at 03:35:40PM +0200, Michael Kerrisk (man-pages) wrote: >> On Wed, Apr 30, 2014 at 1:17 PM, Michael Kerrisk (man-pages) >> <mtk.manpages@gmail.com> wrote: >> > Hello Ondřej, >> > >> > On 04/30/2014 11:07 AM, Ondřej Bílka wrote: >> >> On Wed, Apr 30, 2014 at 10:28:53AM +0200, Michael Kerrisk (man-pages) wrote: >> >>> On 04/28/2014 07:01 PM, David Miller wrote: >> >>>> From: Ondřej Bílka <neleai@seznam.cz> >> >>>> Date: Mon, 28 Apr 2014 18:04:20 +0200 >> >>>> >> >>>>> * sysdeps/unix/sysv/linux/recvmmsg.c (recvmmsg): Do not clobber >> >>>>> timeout argument. >> >>>> >> >>>> It is extremely unfortunate if we've defined this argument as const, >> >>>> now you are making it so that the user has no mechanism to get the >> >>>> updated timeval other than to define their own syscall stubs. >> >>>> >> >>>> This is doubly unfortunately since there is absolutely no reason for >> >>>> us to have defined the interface different from what the kernel >> >>>> actually provides. >> >>> >> >>> Agreed. Surely, the right fix here is simply to remove the "const" >> >>> qualifier from the glibc API? >> >>> >> >> Now I am also for removing const. Initially I looked to documentation >> >> and this was undocumented feature so I was not sure about it. >> > >> > Yes, my mistake. >> >> Sigh! Now I remember why that timeout behavior didn't get documented: >> >> http://thread.gmane.org/gmane.linux.man/3477/focus=3500 >> >> The timeout argument is completely broken, AFAICT. I never did get a >> reply from Arnaldo (but, as you can see in the thread, others agreed >> tha there's a problem), and then I got distracted :-{. > > Indeed, this looks like a big problem and I don't see any viable > solution. I think the documented behavior, for now, should be that the > behavior is unspecified unless the timeout is a null pointer. I got the formulation of the program slightly wrong, but the fundamental problem is the same: http://thread.gmane.org/gmane.linux.man/5677/focus=5702 Cheers, Michael
diff --git a/sysdeps/unix/sysv/linux/recvmmsg.c b/sysdeps/unix/sysv/linux/recvmmsg.c index 57ddf31..04a065f 100644 --- a/sysdeps/unix/sysv/linux/recvmmsg.c +++ b/sysdeps/unix/sysv/linux/recvmmsg.c @@ -35,14 +35,16 @@ #ifdef __NR_recvmmsg int recvmmsg (int fd, struct mmsghdr *vmessages, unsigned int vlen, int flags, - const struct timespec *tmo) + const struct timespec *_tmo) { + struct timespec tmo = *_tmo; + if (SINGLE_THREAD_P) - return INLINE_SYSCALL (recvmmsg, 5, fd, vmessages, vlen, flags, tmo); + return INLINE_SYSCALL (recvmmsg, 5, fd, vmessages, vlen, flags, &tmo); int oldtype = LIBC_CANCEL_ASYNC (); - int result = INLINE_SYSCALL (recvmmsg, 5, fd, vmessages, vlen, flags, tmo); + int result = INLINE_SYSCALL (recvmmsg, 5, fd, vmessages, vlen, flags, &tmo); LIBC_CANCEL_RESET (oldtype); @@ -52,18 +54,20 @@ recvmmsg (int fd, struct mmsghdr *vmessages, unsigned int vlen, int flags, # ifndef __ASSUME_RECVMMSG_SOCKETCALL extern int __internal_recvmmsg (int fd, struct mmsghdr *vmessages, unsigned int vlen, int flags, - const struct timespec *tmo) + struct timespec *tmo) attribute_hidden; static int have_recvmmsg; int recvmmsg (int fd, struct mmsghdr *vmessages, unsigned int vlen, int flags, - const struct timespec *tmo) + const struct timespec *_tmo) { + struct timespec tmo = *_tmo; + if (__glibc_likely (have_recvmmsg >= 0)) { - int ret = __internal_recvmmsg (fd, vmessages, vlen, flags, tmo); + int ret = __internal_recvmmsg (fd, vmessages, vlen, flags, &tmo); /* The kernel returns -EINVAL for unknown socket operations. We need to convert that error to an ENOSYS error. */ if (__builtin_expect (ret < 0, 0)