Message ID | 1397373978-17449-1-git-send-email-peter@peter-b.co.uk |
---|---|
State | New |
Headers | show |
Peter TB Brett <peter@peter-b.co.uk> writes:
> + || (statfs64 (_PATH_DEVPTS, &fsbuf) == 0
This is not namespace clean, and will introduce a PLT call (you did run
make check?).
Andreas.
On 2014-04-13 08:44, Andreas Schwab wrote: > Peter TB Brett <peter@peter-b.co.uk> writes: > >> + || (statfs64 (_PATH_DEVPTS, &fsbuf) == 0 > > This is not namespace clean, and will introduce a PLT call (you did run > make check?). I specifically asked about this on libc-help yesterday: http://thread.gmane.org/gmane.comp.lib.glibc.user/2196 I got the following answer: On 2014-04-12 18:02, Carlos O'Donell wrote: > > To make a long story short you should just call statfs64. Some further advice and/or explanation would be appreciated. Peter
On 2014-04-13 08:44, Andreas Schwab wrote: > Peter TB Brett <peter@peter-b.co.uk> writes: > >> + || (statfs64 (_PATH_DEVPTS, &fsbuf) == 0 > > This is not namespace clean, and will introduce a PLT call (you did run > make check?). I just re-ran `make check' to verify -- and received no output relating to statfs. Which specific test am I supposed to be looking at? Peter
On Sun, Apr 13, 2014 at 09:44:11AM +0200, Andreas Schwab wrote: > Peter TB Brett <peter@peter-b.co.uk> writes: > > > + || (statfs64 (_PATH_DEVPTS, &fsbuf) == 0 > > This is not namespace clean, and will introduce a PLT call (you did run > make check?). > According to Carlos its part of librt.so and indirection is needed there, see https://www.sourceware.org/ml/libc-help/2014-04/msg00024.html
Ondřej Bílka <neleai@seznam.cz> writes: > On Sun, Apr 13, 2014 at 09:44:11AM +0200, Andreas Schwab wrote: >> Peter TB Brett <peter@peter-b.co.uk> writes: >> >> > + || (statfs64 (_PATH_DEVPTS, &fsbuf) == 0 >> >> This is not namespace clean, and will introduce a PLT call (you did run >> make check?). >> > According to Carlos its part of librt.so ??? posix_openpt is part of libc. Andreas.
On Sun 13 Apr 2014 19:07:49 Andreas Schwab wrote: > Ondřej Bílka <neleai@seznam.cz> writes: > > On Sun, Apr 13, 2014 at 09:44:11AM +0200, Andreas Schwab wrote: > >> Peter TB Brett <peter@peter-b.co.uk> writes: > >> > + || (statfs64 (_PATH_DEVPTS, &fsbuf) == 0 > >> > >> This is not namespace clean, and will introduce a PLT call (you did run > >> make check?). > > > > According to Carlos its part of librt.so > > ??? posix_openpt is part of libc. right, posix_openpt is part of libc, but shm_open is part of librt. so the former should use __statfs64 to avoid the PLT while the latter should use statfs64. so can we get an updated patch now ? :) -mike
On Sat, 2 Aug 2014, Mike Frysinger wrote: > right, posix_openpt is part of libc, but shm_open is part of librt. so the > former should use __statfs64 to avoid the PLT while the latter should use > statfs64. No, both should use __statfs64 as statfs64 is not part of the POSIX namespace but shm_open is. As previously discussed, PLT avoidance and being namespace-clean are separate issues. That does of course mean __statfs64 needs to be exported from libc.so. Although you could use a GLIBC_PRIVATE export (and that's the conservative approach at this development stage), eventually we'll want such exports at public symbol versions as part of a fix for bug 14106. (statfs isn't part of a standard, so the need for such a public export is a bit less clear, but it still seems appropriate given that you can use <sys/statfs.h> without _LARGEFILE64_SOURCE being defined, and in that case it seems reasonable to consider statfs64 part of the user's namespace.)
On Mon 04 Aug 2014 15:51:15 Joseph S. Myers wrote: > On Sat, 2 Aug 2014, Mike Frysinger wrote: > > right, posix_openpt is part of libc, but shm_open is part of librt. so > > the > > former should use __statfs64 to avoid the PLT while the latter should use > > statfs64. > > No, both should use __statfs64 as statfs64 is not part of the POSIX > namespace but shm_open is. As previously discussed, PLT avoidance and > being namespace-clean are separate issues. why does that matter ? statfs64 is always exported by libc and thus is in the visible ABI namespace. i don't see why glibc's internals need to be concerned with conforming to POSIX at all -- they're internals. what are you trying to cater to ? -mike
On Mon, Aug 4, 2014 at 5:49 PM, Mike Frysinger <vapier@gentoo.org> wrote: > On Mon 04 Aug 2014 15:51:15 Joseph S. Myers wrote: >> On Sat, 2 Aug 2014, Mike Frysinger wrote: >> > right, posix_openpt is part of libc, but shm_open is part of librt. so >> > the >> > former should use __statfs64 to avoid the PLT while the latter should use >> > statfs64. >> >> No, both should use __statfs64 as statfs64 is not part of the POSIX >> namespace but shm_open is. As previously discussed, PLT avoidance and >> being namespace-clean are separate issues. > > why does that matter ? statfs64 is always exported by libc and thus is in the > visible ABI namespace. i don't see why glibc's internals need to be concerned > with conforming to POSIX at all -- they're internals. what are you trying to > cater to ? A program can define a variable called statfs64 which is used instead of the libc.so one when calling from librt.so. Thanks, Andrew Pinski > -mike
On Mon 04 Aug 2014 18:06:58 Andrew Pinski wrote: > On Mon, Aug 4, 2014 at 5:49 PM, Mike Frysinger wrote: > > On Mon 04 Aug 2014 15:51:15 Joseph S. Myers wrote: > >> On Sat, 2 Aug 2014, Mike Frysinger wrote: > >> > right, posix_openpt is part of libc, but shm_open is part of librt. so > >> > the > >> > former should use __statfs64 to avoid the PLT while the latter should > >> > use > >> > statfs64. > >> > >> No, both should use __statfs64 as statfs64 is not part of the POSIX > >> namespace but shm_open is. As previously discussed, PLT avoidance and > >> being namespace-clean are separate issues. > > > > why does that matter ? statfs64 is always exported by libc and thus is in > > the visible ABI namespace. i don't see why glibc's internals need to be > > concerned with conforming to POSIX at all -- they're internals. what are > > you trying to cater to ? > > A program can define a variable called statfs64 which is used instead > of the libc.so one when calling from librt.so. librt/libpthread/etc... have lots of calls to funcs from libc which can be classified in the same way. they aren't going through GLIBC_PRIVATE nor using __ prefixes. what makes this any different ? -mike
On Mon 04 Aug 2014 23:50:40 Mike Frysinger wrote: > On Mon 04 Aug 2014 18:06:58 Andrew Pinski wrote: > > On Mon, Aug 4, 2014 at 5:49 PM, Mike Frysinger wrote: > > > On Mon 04 Aug 2014 15:51:15 Joseph S. Myers wrote: > > >> On Sat, 2 Aug 2014, Mike Frysinger wrote: > > >> > right, posix_openpt is part of libc, but shm_open is part of librt. > > >> > so > > >> > the > > >> > former should use __statfs64 to avoid the PLT while the latter should > > >> > use > > >> > statfs64. > > >> > > >> No, both should use __statfs64 as statfs64 is not part of the POSIX > > >> namespace but shm_open is. As previously discussed, PLT avoidance and > > >> being namespace-clean are separate issues. > > > > > > why does that matter ? statfs64 is always exported by libc and thus is > > > in > > > the visible ABI namespace. i don't see why glibc's internals need to be > > > concerned with conforming to POSIX at all -- they're internals. what > > > are > > > you trying to cater to ? > > > > A program can define a variable called statfs64 which is used instead > > of the libc.so one when calling from librt.so. > > librt/libpthread/etc... have lots of calls to funcs from libc which can be > classified in the same way. they aren't going through GLIBC_PRIVATE nor > using __ prefixes. what makes this any different ? just caught up on another thread from while i was gone. Carlos wrote up a nice explanation here: https://sourceware.org/glibc/wiki/Style_and_Conventions#Double-underscore_names_for_public_API_functions so the answer to the other examples i highlighted is that they're wrong too :) -mike
On Mon, Aug 04, 2014 at 06:06:58PM -0700, Andrew Pinski wrote: > On Mon, Aug 4, 2014 at 5:49 PM, Mike Frysinger <vapier@gentoo.org> wrote: > > On Mon 04 Aug 2014 15:51:15 Joseph S. Myers wrote: > >> On Sat, 2 Aug 2014, Mike Frysinger wrote: > >> > right, posix_openpt is part of libc, but shm_open is part of librt. so > >> > the > >> > former should use __statfs64 to avoid the PLT while the latter should use > >> > statfs64. > >> > >> No, both should use __statfs64 as statfs64 is not part of the POSIX > >> namespace but shm_open is. As previously discussed, PLT avoidance and > >> being namespace-clean are separate issues. > > > > why does that matter ? statfs64 is always exported by libc and thus is in the > > visible ABI namespace. i don't see why glibc's internals need to be concerned > > with conforming to POSIX at all -- they're internals. what are you trying to > > cater to ? > > > A program can define a variable called statfs64 which is used instead > of the libc.so one when calling from librt.so. While according to POSIX this is true (and actually statfs64 is a really bad example since even statfs isn't POSIX), in general making your own symbols that clash with the *64 names really can't work with glibc at all, at least not on 32-bit systems. -D_FILE_OFFSET_BITS=64 is basically mandatory on 32-bit systems for many functions to work since modern systems have 64-bit inode numbers and all sorts of things fail without it. Isn't it about time glibc added namespace-safe names for these functions (e.g. __open64, etc.) and changed the headers to redirect to these namespace-safe names rather than using the old ones? Rich
diff --git a/ChangeLog b/ChangeLog index a42e08e..4e52aa8 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,12 @@ +2014-04-13 Peter Brett <peter@peter-b.co.uk> + + [BZ 15514] + * sysdeps/unix/sysv/linux/shm_open.c (where_is_shmfs): Check for + shmfs using statfs64(). + * sysdeps/unix/sysv/linux/getpt.c (__posix_openpt): Check for + /dev/pts and devfs using statfs64(). + Reported by Philip Guenther <guenther@gmail.com> + 2014-04-12 Allan McRae <allan@archlinux.org> [BZ #16838] diff --git a/sysdeps/unix/sysv/linux/getpt.c b/sysdeps/unix/sysv/linux/getpt.c index cea2fa6..c8f9275 100644 --- a/sysdeps/unix/sysv/linux/getpt.c +++ b/sysdeps/unix/sysv/linux/getpt.c @@ -46,15 +46,15 @@ __posix_openpt (oflag) fd = __open (_PATH_DEVPTMX, oflag); if (fd != -1) { - struct statfs fsbuf; + struct statfs64 fsbuf; static int devpts_mounted; /* Check that the /dev/pts filesystem is mounted or if /dev is a devfs filesystem (this implies /dev/pts). */ if (devpts_mounted - || (__statfs (_PATH_DEVPTS, &fsbuf) == 0 + || (statfs64 (_PATH_DEVPTS, &fsbuf) == 0 && fsbuf.f_type == DEVPTS_SUPER_MAGIC) - || (__statfs (_PATH_DEV, &fsbuf) == 0 + || (statfs64 (_PATH_DEV, &fsbuf) == 0 && fsbuf.f_type == DEVFS_SUPER_MAGIC)) { /* Everything is ok. */ diff --git a/sysdeps/unix/sysv/linux/shm_open.c b/sysdeps/unix/sysv/linux/shm_open.c index cec6fdb..a41b6b5 100644 --- a/sysdeps/unix/sysv/linux/shm_open.c +++ b/sysdeps/unix/sysv/linux/shm_open.c @@ -55,14 +55,14 @@ static void where_is_shmfs (void) { char buf[512]; - struct statfs f; + struct statfs64 f; struct mntent resmem; struct mntent *mp; FILE *fp; /* The canonical place is /dev/shm. This is at least what the documentation tells everybody to do. */ - if (__statfs (defaultdir, &f) == 0 && (f.f_type == SHMFS_SUPER_MAGIC + if (statfs64 (defaultdir, &f) == 0 && (f.f_type == SHMFS_SUPER_MAGIC || f.f_type == RAMFS_MAGIC)) { /* It is in the normal place. */ @@ -97,7 +97,7 @@ where_is_shmfs (void) /* First make sure this really is the correct entry. At least some versions of the kernel give wrong information because of the implicit mount of the shmfs for SysV IPC. */ - if (__statfs (mp->mnt_dir, &f) != 0 || (f.f_type != SHMFS_SUPER_MAGIC + if (statfs64 (mp->mnt_dir, &f) != 0 || (f.f_type != SHMFS_SUPER_MAGIC && f.f_type != RAMFS_MAGIC)) continue;