diff mbox

[v2,BZ,16852] Do not clobber recvmmsg argument.

Message ID 20140428160420.GA23142@domone.podge
State Superseded
Headers

Commit Message

Ondrej Bilka April 28, 2014, 4:04 p.m. UTC 
  On Mon, Apr 28, 2014 at 05:40:56PM +0200, Andreas Schwab wrote:
> Ondřej Bílka <neleai@seznam.cz> writes:
> 
> > +  struct timespec dummy;
> > +  memcpy (&dummy, tmo, sizeof (struct timespec));
> 
> What's wrong with an assignment?
>
My first idea was memcpy, thanks. 
> >    if (SINGLE_THREAD_P)
> >      return INLINE_SYSCALL (recvmmsg, 5, fd, vmessages, vlen, flags, tmo);
> 
> You still pass a const where a non-const is expected.
>
fixed. 
> Andreas.
> 

Here is v2.

	* sysdeps/unix/sysv/linux/recvmmsg.c (recvmmsg): Do not clobber
	timeout argument.

Comments

David Miller April 28, 2014, 5:01 p.m. UTC | #1 
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.
Rich Felker April 28, 2014, 6:06 p.m. UTC | #2 
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
Michael Kerrisk (man-pages) April 30, 2014, 8:28 a.m. UTC | #3 
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
Ondrej Bilka April 30, 2014, 9:07 a.m. UTC | #4 
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?
Michael Kerrisk \(man-pages\) April 30, 2014, 11:17 a.m. UTC | #5 
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
Michael Kerrisk \(man-pages\) April 30, 2014, 1:35 p.m. UTC | #6 
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
Rich Felker April 30, 2014, 4:04 p.m. UTC | #7 
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
Michael Kerrisk \(man-pages\) May 4, 2014, 7:31 p.m. UTC | #8 
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 mbox

Patch

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)