Patchwork Use statfs64() in shm_open() and posix_getpt().

login
register
mail settings
Submitter Peter TB Brett
Date April 13, 2014, 7:26 a.m.
Message ID <1397373978-17449-1-git-send-email-peter@peter-b.co.uk>
Download mbox | patch
Permalink /patch/525/
State Changes Requested
Headers show

Comments

Peter TB Brett - April 13, 2014, 7:26 a.m.
Philip Guenther noted in [BZ 15514] that statfs() may EOVERFLOW when
running 32-bit programs on 64-bit systems, and that shm_open() and
posix_getpt() may be affected.

This patch modifies shm_open() and posix_getpt() to use statfs64()
instead of __statfs().

Reported-by: Philip Guenther <guenther@gmail.com>
---
 ChangeLog                          | 9 +++++++++
 sysdeps/unix/sysv/linux/getpt.c    | 6 +++---
 sysdeps/unix/sysv/linux/shm_open.c | 6 +++---
 3 files changed, 15 insertions(+), 6 deletions(-)
Andreas Schwab - April 13, 2014, 7:44 a.m.
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.
Peter TB Brett - April 13, 2014, 8:09 a.m.
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
Peter TB Brett - April 13, 2014, 8:41 a.m.
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
Ondrej Bilka - April 13, 2014, 11:15 a.m.
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
Andreas Schwab - April 13, 2014, 5:07 p.m.
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.
Mike Frysinger - Aug. 2, 2014, 2:30 p.m.
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
Joseph Myers - Aug. 4, 2014, 3:51 p.m.
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.)
Mike Frysinger - Aug. 5, 2014, 12:49 a.m.
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
pinskia@gmail.com - Aug. 5, 2014, 1:06 a.m.
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
Mike Frysinger - Aug. 5, 2014, 3:50 a.m.
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
Mike Frysinger - Aug. 5, 2014, 4:50 a.m.
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
Rich Felker - Aug. 5, 2014, 4:14 p.m.
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

Patch

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;