Message ID | 1410305478.27502.10.camel@bordewijk.wildebeest.org |
---|---|
State | Committed |
Headers |
Received: (qmail 6332 invoked by alias); 9 Sep 2014 23:31:26 -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-##L=##H@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 6317 invoked by uid 89); 9 Sep 2014 23:31:25 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-4.9 required=5.0 tests=AWL, BAYES_00, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Message-ID: <1410305478.27502.10.camel@bordewijk.wildebeest.org> Subject: Bug 16713 - [s390x] Member "sa_flags" does not have the correct type From: Mark Wielaard <mjw@redhat.com> To: Stefan Liebler <stli@linux.vnet.ibm.com> Cc: libc-alpha@sourceware.org Date: Wed, 10 Sep 2014 01:31:18 +0200 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Mime-Version: 1.0 |
Commit Message
Mark Wielaard
Sept. 9, 2014, 11:31 p.m. UTC
Hi, [Sorry for the duplicate Stefan, I got the list name wrong on the CC.] To fix the following bug: https://sourceware.org/bugzilla/show_bug.cgi?id=16713 There was this simple change: But means glibc and the kernel don't agree anymore on the structure layout of struct sigaction. The kernel is still using unsigned long sa_flags on s390. I guess this still works out because all current SA_... flag values fit in an int. But it does mean the kernel cannot expand the flag field values anymore in the future. Because valgrind follows the kernel definition the above change causes valgrind memcheck to complain now whenever only the sa_flags value (as seen by user space) is assigned. For example in nptl-init.c: struct sigaction sa; sa.sa_sigaction = sigcancel_handler; sa.sa_flags = SA_SIGINFO; __sigemptyset (&sa.sa_mask); (void) __libc_sigaction (SIGCANCEL, &sa, NULL); valgrind will now warn: Syscall param rt_sigaction(act->sa_flags) points to uninitialised byte(s) at 0x42EECC8: __libc_sigaction (sigaction.c:42) by 0x42E2253: __pthread_initialize_minimal (nptl-init.c:381) by 0x42E15EF: ??? (in /usr/lib64/libpthread-2.19.90.so) Because the '__glibc_reserved0' part of sa_flags as the kernel will see them is indeed undefined. We can easily change valgrind to follow the layout as now used by glibc/user space on s390 instead of the kernel layout and just ignore the __glibc_reserved0 field. But I wanted to first make sure this change/difference between glibc/kernel was intended. Maybe glibc wants to pass the struct as intended by the kernel and needs to create a "bridge" struct that properly initializes the kernel's view of sa_flags? Thanks, Mark
Comments
On Tuesday, September 09 2014, Mark Wielaard wrote: > Maybe glibc wants to pass the struct as intended by the kernel and needs > to create a "bridge" struct that properly initializes the kernel's view > of sa_flags? BTW, as discussed with Mark, a similar approach is done in GDB already. However, I think the question is: does glibc need to have both declarations? IIUC, this difference was introduced because of POSIX conformance, but if it does more harm than good... Anyway, I'm really out of my scope here. Just my 0.0001 cent.
On Tue, Sep 09, 2014 at 08:49:22PM -0400, Sergio Durigan Junior wrote: > On Tuesday, September 09 2014, Mark Wielaard wrote: > > > Maybe glibc wants to pass the struct as intended by the kernel and needs > > to create a "bridge" struct that properly initializes the kernel's view > > of sa_flags? > > BTW, as discussed with Mark, a similar approach is done in GDB already. > > However, I think the question is: does glibc need to have both > declarations? IIUC, this difference was introduced because of POSIX > conformance, but if it does more harm than good... > > Anyway, I'm really out of my scope here. Just my 0.0001 cent. glibc needs to have a POSIX conforming public definition of struct sigaction. If there's any risk of the kernel interpreting the junk in the upper bits due to the kernel's incorrect definition of the structure, then glibc needs to make a copy to pass to the kernel, with the unused bits filled with zeros in the copy. Rich
Hi, yes the kernel and glibc struct differs on s390. This is intended in order to get POSIX conform. I got kernel agreement before i changed the struct. The kernel has no problem with the __glibc_reserved0 variable and its random content. Bye Stefan On 09/10/2014 01:31 AM, Mark Wielaard wrote: > Hi, > > [Sorry for the duplicate Stefan, I got the list name wrong on the CC.] > > To fix the following bug: > https://sourceware.org/bugzilla/show_bug.cgi?id=16713 > There was this simple change: > > --- a/sysdeps/unix/sysv/linux/s390/bits/sigaction.h > +++ b/sysdeps/unix/sysv/linux/s390/bits/sigaction.h > @@ -43,7 +43,8 @@ struct sigaction > #endif > > /* Special flags. */ > - unsigned long int sa_flags; > + int __glibc_reserved0; > + int sa_flags; > > /* Restore handler. */ > void (*sa_restorer) (void); > > But means glibc and the kernel don't agree anymore on the structure > layout of struct sigaction. The kernel is still using unsigned long > sa_flags on s390. I guess this still works out because all current > SA_... flag values fit in an int. But it does mean the kernel cannot > expand the flag field values anymore in the future. > > Because valgrind follows the kernel definition the above change causes > valgrind memcheck to complain now whenever only the sa_flags value (as > seen by user space) is assigned. For example in nptl-init.c: > > struct sigaction sa; > sa.sa_sigaction = sigcancel_handler; > sa.sa_flags = SA_SIGINFO; > __sigemptyset (&sa.sa_mask); > > (void) __libc_sigaction (SIGCANCEL, &sa, NULL); > > valgrind will now warn: > > Syscall param rt_sigaction(act->sa_flags) points to uninitialised byte(s) > at 0x42EECC8: __libc_sigaction (sigaction.c:42) > by 0x42E2253: __pthread_initialize_minimal (nptl-init.c:381) > by 0x42E15EF: ??? (in /usr/lib64/libpthread-2.19.90.so) > > Because the '__glibc_reserved0' part of sa_flags as the kernel will see > them is indeed undefined. > > We can easily change valgrind to follow the layout as now used by > glibc/user space on s390 instead of the kernel layout and just ignore > the __glibc_reserved0 field. But I wanted to first make sure this > change/difference between glibc/kernel was intended. > > Maybe glibc wants to pass the struct as intended by the kernel and needs > to create a "bridge" struct that properly initializes the kernel's view > of sa_flags? > > Thanks, > > Mark > >
On 09/10/2014 09:18 AM, Stefan Liebler wrote: > yes the kernel and glibc struct differs on s390. This is intended in > order to get POSIX conform. I got kernel agreement before i changed the > struct. The kernel has no problem with the __glibc_reserved0 variable > and its random content. But valgrind will need a new suppression for this, I think.
On Wed, 2014-09-10 at 09:18 +0200, Stefan Liebler wrote: > yes the kernel and glibc struct differs on s390. This is intended in > order to get POSIX conform. I got kernel agreement before i changed the > struct. The kernel has no problem with the __glibc_reserved0 variable > and its random content. Thanks. I'll change valgrind accordingly then on s390 to ignore those undefined bytes in the sigaction struct: https://bugs.kde.org/show_bug.cgi?id=338974 Cheers, Mark
On Wed, 10 Sep 2014, Mark Wielaard wrote: > But means glibc and the kernel don't agree anymore on the structure > layout of struct sigaction. The kernel is still using unsigned long > sa_flags on s390. I guess this still works out because all current > SA_... flag values fit in an int. But it does mean the kernel cannot > expand the flag field values anymore in the future. Given that there are platforms where the flags field is 32-bit in the kernel, and that these bits seem unlikely to be architecture-specific, there wasn't really scope for expanding these values beyond 32-bit anyway.
On Tuesday, September 09 2014, Rich Felker wrote: > glibc needs to have a POSIX conforming public definition of struct > sigaction. If there's any risk of the kernel interpreting the junk in > the upper bits due to the kernel's incorrect definition of the > structure, then glibc needs to make a copy to pass to the kernel, with > the unused bits filled with zeros in the copy. Thanks for the explanation. Then yeah, the "bridge struct" idea seems like the best one.
--- a/sysdeps/unix/sysv/linux/s390/bits/sigaction.h +++ b/sysdeps/unix/sysv/linux/s390/bits/sigaction.h @@ -43,7 +43,8 @@ struct sigaction #endif /* Special flags. */ - unsigned long int sa_flags; + int __glibc_reserved0; + int sa_flags; /* Restore handler. */ void (*sa_restorer) (void);