[1/3] Mark ld.so internel mmap functions hidden

Message ID CAMe9rOo0cs_bDbKo1+-+ObeyDd09SUrPaYpCR7ACq0x0-f-usw@mail.gmail.com
State New, archived
Headers

Commit Message

H.J. Lu Oct. 19, 2015, 7:31 p.m. UTC
  On Mon, Oct 19, 2015 at 11:31 AM, Roland McGrath <roland@hack.frob.com> wrote:
> NAK to #ifdef in generic code.

Here is the updated patch with a new header file, dl-mman.h.
OK for master?
  

Comments

Richard Henderson Oct. 19, 2015, 8:45 p.m. UTC | #1
On 10/19/2015 09:31 AM, H.J. Lu wrote:
> +# if IS_IN (rtld)
> +#  include <dl-mman.h>
> +extern __typeof (__mprotect) __mprotect attribute_hidden;
> +extern __typeof (__munmap) __munmap attribute_hidden;

Surely if mmap can't be hidden, then munmap can't either.

Surely you should just put all of these within dl-mman.h, even if in the end 
there are a few lines of overlap.


r~
  
H.J. Lu Oct. 19, 2015, 9:04 p.m. UTC | #2
On Mon, Oct 19, 2015 at 1:45 PM, Richard Henderson <rth@twiddle.net> wrote:
> On 10/19/2015 09:31 AM, H.J. Lu wrote:
>>
>> +# if IS_IN (rtld)
>> +#  include <dl-mman.h>
>> +extern __typeof (__mprotect) __mprotect attribute_hidden;
>> +extern __typeof (__munmap) __munmap attribute_hidden;
>
>
> Surely if mmap can't be hidden, then munmap can't either.
>
> Surely you should just put all of these within dl-mman.h, even if in the end
> there are a few lines of overlap.
>

sysdeps/mach/hurd/Versions has

ld {
  GLIBC_2.0 {
    # variables that must be shared with libc
    __hurd_sigthread_stack_base; __hurd_sigthread_stack_end;
    __hurd_sigthread_variables;
    __hurd_threadvar_stack_mask;  __hurd_threadvar_stack_offset;

    # functions that must be shared with libc
    __close; __getcwd; __getpid;
    __mmap; __open; __xstat64; __fxstat64;
    _exit; _hurd_intr_rpc_mach_msg;
    abort;
  }
  GLIBC_2.2.6 {
    # this also must be shared with libc.
    __errno_location;
  }
  GLIBC_PRIVATE {
    _dl_init_first;

    # functions that must be shared with libc
    __libc_read; __libc_write; __libc_lseek64;
  }
}

__mprotect and __munmap are hidden in ld.so on Hurd.
  
Roland McGrath Oct. 19, 2015, 10:19 p.m. UTC | #3
> On 10/19/2015 09:31 AM, H.J. Lu wrote:
> > +# if IS_IN (rtld)
> > +#  include <dl-mman.h>
> > +extern __typeof (__mprotect) __mprotect attribute_hidden;
> > +extern __typeof (__munmap) __munmap attribute_hidden;
> 
> Surely if mmap can't be hidden, then munmap can't either.

Actually, that's not true.  On the Hurd, the mmap in ld.so is a special
hack different from the real libc implementation because it has to handle
the file descriptor argument.  munmap and mprotect have trivial
implementations that just use microkernel calls, so using the copies in
ld.so is fine (and might as well avoid the PLT when you can).

> Surely you should just put all of these within dl-mman.h, even if in the
> end there are a few lines of overlap.

Agreed.  The details I just explained are very Hurd-specific, and it seems
sensible that the Hurd file declare as hidden the ones that can be because
of that Hurd-specific rationale.
  
Roland McGrath Oct. 19, 2015, 10:20 p.m. UTC | #4
All new files should have a descriptive comment at the top.  For any new
sysdeps file, the generic version should have clear comments saying what
the purpose of the file is and what sysdeps variants are expected to do.
  

Patch

From d35367c4ac22d9873f9643018be0326b31bc2987 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Wed, 14 Oct 2015 15:14:15 -0700
Subject: [PATCH 1/2] Mark ld.so internel mmap functions hidden

Since ld.so internel mmap functions are only used internally in ld.so,
they can be made hidden.  Don't hide __mmap on Hurd, since __mmap in
ld.so will be preempted by the one in libc.so after bootstrap.

	 [BZ #19122]
	 * include/sys/mman.h [IS_IN (rtld)]: Include <dl-mman.h>.
	 [IS_IN (rtld)] (__mprotect): Add attribute_hidden.
	 [IS_IN (rtld)] (__munmap): Likewise.
	 * sysdeps/generic/dl-mman.h: New file.
	 * sysdeps/mach/hurd/dl-mman.h: Likewise.
---
 include/sys/mman.h          |  6 ++++++
 sysdeps/generic/dl-mman.h   | 18 ++++++++++++++++++
 sysdeps/mach/hurd/dl-mman.h | 19 +++++++++++++++++++
 3 files changed, 43 insertions(+)
 create mode 100644 sysdeps/generic/dl-mman.h
 create mode 100644 sysdeps/mach/hurd/dl-mman.h

diff --git a/include/sys/mman.h b/include/sys/mman.h
index fd125ec..e112591 100644
--- a/include/sys/mman.h
+++ b/include/sys/mman.h
@@ -16,6 +16,12 @@  libc_hidden_proto (__madvise)
 /* This one is Linux specific.  */
 extern void *__mremap (void *__addr, size_t __old_len,
 		       size_t __new_len, int __flags, ...);
+
+# if IS_IN (rtld)
+#  include <dl-mman.h>
+extern __typeof (__mprotect) __mprotect attribute_hidden;
+extern __typeof (__munmap) __munmap attribute_hidden;
+# endif
 #endif
 
 #endif
diff --git a/sysdeps/generic/dl-mman.h b/sysdeps/generic/dl-mman.h
new file mode 100644
index 0000000..5b44d3a
--- /dev/null
+++ b/sysdeps/generic/dl-mman.h
@@ -0,0 +1,18 @@ 
+/* Copyright (C) 2015 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+extern __typeof (__mmap) __mmap attribute_hidden;
diff --git a/sysdeps/mach/hurd/dl-mman.h b/sysdeps/mach/hurd/dl-mman.h
new file mode 100644
index 0000000..8821da5
--- /dev/null
+++ b/sysdeps/mach/hurd/dl-mman.h
@@ -0,0 +1,19 @@ 
+/* Copyright (C) 2015 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+/* Can't hide __mmap on Hurd, since __mmap in ld.so will be preempted by
+   the one in libc.so after bootstrap.  */
-- 
2.4.3