Message ID | 20140428160420.GA23142@domone.podge |
---|---|
State | Superseded |
Headers |
Return-Path: <x14307373@homiemail-mx21.g.dreamhost.com> X-Original-To: siddhesh@wilcox.dreamhost.com Delivered-To: siddhesh@wilcox.dreamhost.com Received: from homiemail-mx21.g.dreamhost.com (peon2454.g.dreamhost.com [208.113.200.127]) by wilcox.dreamhost.com (Postfix) with ESMTP id C124D360060 for <siddhesh@wilcox.dreamhost.com>; Mon, 28 Apr 2014 09:04:31 -0700 (PDT) Received: by homiemail-mx21.g.dreamhost.com (Postfix, from userid 14307373) id 74B0013F8992; Mon, 28 Apr 2014 09:04:30 -0700 (PDT) X-Original-To: glibc@patchwork.siddhesh.in Delivered-To: x14307373@homiemail-mx21.g.dreamhost.com Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by homiemail-mx21.g.dreamhost.com (Postfix) with ESMTPS id 6043513DF3A1 for <glibc@patchwork.siddhesh.in>; Mon, 28 Apr 2014 09:04:29 -0700 (PDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:date:from:to:cc:subject:message-id:references :mime-version:content-type:content-transfer-encoding :in-reply-to; q=dns; s=default; b=faHE70fshuqbvYxP3llg5QYQIiujiN j65MPWYB38jUldjkxHyZosXt17vkDQcXp8JDW90LNuNXQFkw7z/22EYt219f5wxz zb+J1yPPge/Bo6L+yhvB7vbudqoIM0RlF56jX6u/vBURe5Ykn69fC3AaKGYoXKDi If67p5voDxwsM= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:date:from:to:cc:subject:message-id:references :mime-version:content-type:content-transfer-encoding :in-reply-to; s=default; bh=DQhnHxdCMCZu5K1JVr3U59y3M9I=; b=Gruo MA2CjlL3eKUGg10pv9cqIZcMFNxbiV9RsI1t4qQQlX/yXzVZhZqhT6ABzPeXf0JO hIRTvaTLS25fLTTgm8fOZP/riKh9Wa98DNDWn3Sob5QWDuS8Nsu3xij5iL9O4DOl AArLhdgy7f0IvZSj4EuLDO63wQW7V+4ruEyDy+0= Received: (qmail 15789 invoked by alias); 28 Apr 2014 16:04:27 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-glibc=patchwork.siddhesh.in@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 15778 invoked by uid 89); 28 Apr 2014 16:04:26 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.6 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, SPF_NEUTRAL autolearn=no version=3.3.2 X-HELO: popelka.ms.mff.cuni.cz Date: Mon, 28 Apr 2014 18:04:20 +0200 From: =?utf-8?B?T25kxZllaiBCw61sa2E=?= <neleai@seznam.cz> To: Andreas Schwab <schwab@linux-m68k.org> Cc: libc-alpha@sourceware.org Subject: [PATCH v2][BZ 16852] Do not clobber recvmmsg argument. Message-ID: <20140428160420.GA23142@domone.podge> References: <20140428152937.GA1736@domone.podge> <87mwf5zcl3.fsf@igel.home> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <87mwf5zcl3.fsf@igel.home> User-Agent: Mutt/1.5.20 (2009-06-14) X-DH-Original-To: glibc@patchwork.siddhesh.in |
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
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)