diff mbox series

[hurd,commited] hurd: fix build with -fstack-protector-strong libmachuser and libhurduser also need stack_chk_fail_local and they do not link against libc_nonshared. * mach/stack_chk_fail_local.c: New file. * hurd/stack_chk_fail_local.c: New file. * mach/

Message ID 20170827201245.g5tfjijpxgmgufqz@var.youpi.perso.aquilenet.fr
State New
Headers show
Series [hurd,commited] hurd: fix build with -fstack-protector-strong libmachuser and libhurduser also need stack_chk_fail_local and they do not link against libc_nonshared. * mach/stack_chk_fail_local.c: New file. * hurd/stack_chk_fail_local.c: New file. * mach/ | expand

Commit Message

Samuel Thibault Aug. 27, 2017, 8:12 p.m. UTC

Comments

Joseph Myers Aug. 28, 2017, 11:44 a.m. UTC | #1
On Sun, 27 Aug 2017, Samuel Thibault wrote:

> +	* mach/Versions (GLIBC_2.4): Add __stack_chk_fail.
> +	* hurd/Versions (GLIBC_2.4): Add __stack_chk_fail.

These changes look suspicious.  debug/Versions already exports 
__stack_chk_fail from libc at version GLIBC_2.4, so I'd expect those new 
Versions entries to have no effect (meaning they should be removed as 
useless).  And any export added at the symbol version for a past release 
needs a more detailed justification given, presumably along the lines of 
"unmodified glibc 2.4 and later releases have not built or worked for 
Hurd, so the de facto GLIBC_2.4 ABI is determined by the binaries people 
have actually been using, presumably Debian, which include that symbol at 
that version (and, hopefully, have done so ever since Debian packages of 
2.4)".

(The missing semicolon at the end of the new Versions entries is also 
suspicious; at least, contrary to the syntax normally used in these 
files.)
Samuel Thibault Aug. 28, 2017, 12:23 p.m. UTC | #2
Hello,

Joseph Myers, on lun. 28 août 2017 11:44:56 +0000, wrote:
> On Sun, 27 Aug 2017, Samuel Thibault wrote:
> > +	* mach/Versions (GLIBC_2.4): Add __stack_chk_fail.
> > +	* hurd/Versions (GLIBC_2.4): Add __stack_chk_fail.
> 
> These changes look suspicious.  debug/Versions already exports 
> __stack_chk_fail from libc at version GLIBC_2.4, so I'd expect those new 
> Versions entries to have no effect

Ok, I had misunderstood the use of the similar versions added above
(__mach_msg, __mach_msg_overwrite, etc. which are defined by mach
itself)

> (The missing semicolon at the end of the new Versions entries is also 
> suspicious; at least, contrary to the syntax normally used in these 
> files.)

Indeed. If only I could find time to make master working, I'd be able to
work with it instead of keeping a cumbersome flow of patches between 3
differents repos :/

Thanks for the checks,
Samuel
Joseph Myers Aug. 28, 2017, 12:42 p.m. UTC | #3
On Mon, 28 Aug 2017, Samuel Thibault wrote:

> Hello,
> 
> Joseph Myers, on lun. 28 août 2017 11:44:56 +0000, wrote:
> > On Sun, 27 Aug 2017, Samuel Thibault wrote:
> > > +	* mach/Versions (GLIBC_2.4): Add __stack_chk_fail.
> > > +	* hurd/Versions (GLIBC_2.4): Add __stack_chk_fail.
> > 
> > These changes look suspicious.  debug/Versions already exports 
> > __stack_chk_fail from libc at version GLIBC_2.4, so I'd expect those new 
> > Versions entries to have no effect
> 
> Ok, I had misunderstood the use of the similar versions added above
> (__mach_msg, __mach_msg_overwrite, etc. which are defined by mach
> itself)

Well, I'm not familiar with any Mach-specific/Hurd-specific symbol 
versioning issues.  But as far as I know the following apply generally to 
all ports:

* The contents of a particular public symbol version should be fixed at 
the time of the corresponding release (subject to any questions around 
ports that were completely broken at the time of the release so the 
sources people actually used were something else).

* Only one visible (in a subdirectory used by make) Versions entry for a 
given (library, symbol, version) triple should be needed.  So just the 
debug/Versions entry for __stack_chk_fail at version GLIBC_2.4 in libc.

* If a symbol is added only for internal use between glibc's own 
libraries, not for use by external programs or libraries, it should have 
version GLIBC_PRIVATE.  The contents of GLIBC_PRIVATE *can* change 
arbitrarily between releases (although adding symbols in bug fixes on 
release branches is still problematic, because of the effects on running 
programs when you upgrade a live system and the running program later 
tries to load another glibc library, which now expects a new GLIBC_PRIVATE 
symbol).

* The *.abilist test baselines can be used to verify that past symbol 
versions don't get changed unintentionally.  Hurd of course needs such 
baselines added, which you could verify against existing binaries of 
various different versions to make sure they reflect what the ABI of 
different versions was in practice.
Samuel Thibault Aug. 28, 2017, 12:51 p.m. UTC | #4
Joseph Myers, on lun. 28 août 2017 12:42:29 +0000, wrote:
> * The contents of a particular public symbol version should be fixed at 
> the time of the corresponding release

Sure.

> * Only one visible (in a subdirectory used by make) Versions entry for a 
> given (library, symbol, version) triple should be needed.

Yes, that's what I had misunderstood in the mach/ RPC-needed symbols
case, I thought it was needed to make RPCs access them.

> * If a symbol is added only for internal use between glibc's own 
> libraries, not for use by external programs or libraries, it should have 
> version GLIBC_PRIVATE.

Right.

> * The *.abilist test baselines can be used to verify that past symbol 
> versions don't get changed unintentionally.  Hurd of course needs such 
> baselines added,

We miss that indeed.

Samuel
diff mbox series

Patch

diff --git a/ChangeLog b/ChangeLog
index 1b0f15573a..4d94fee93b 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,6 +1,12 @@ 
 2017-08-27  Samuel Thibault  <samuel.thibault@ens-lyon.org>
 
 	* sysdeps/mach/hurd/bits/sysmacros.h: New file.
+	* mach/stack_chk_fail_local.c: New file.
+	* hurd/stack_chk_fail_local.c: New file.
+	* mach/Machrules ($(interface-library)-routines): Add
+	stack_chk_fail_local.
+	* mach/Versions (GLIBC_2.4): Add __stack_chk_fail.
+	* hurd/Versions (GLIBC_2.4): Add __stack_chk_fail.
 
 2017-08-25  H.J. Lu  <hongjiu.lu@intel.com>
 
diff --git a/hurd/Versions b/hurd/Versions
index 77f5b4271e..011edc7384 100644
--- a/hurd/Versions
+++ b/hurd/Versions
@@ -129,6 +129,10 @@  libc {
     # functions used in macros & inline functions
     __errno_location;
   }
+  GLIBC_2.4 {
+    # functions used by RPC stubs
+    __stack_chk_fail
+  }
 
   HURD_CTHREADS_0.3 {
     # weak refs to libthreads functions that libc calls iff libthreads in use
diff --git a/hurd/stack_chk_fail_local.c b/hurd/stack_chk_fail_local.c
new file mode 100644
index 0000000000..305871fbc0
--- /dev/null
+++ b/hurd/stack_chk_fail_local.c
@@ -0,0 +1 @@ 
+#include <debug/stack_chk_fail_local.c>
diff --git a/mach/Machrules b/mach/Machrules
index 36adfc7681..29114b1eef 100644
--- a/mach/Machrules
+++ b/mach/Machrules
@@ -221,7 +221,7 @@  endif
 
 ifdef interface-library
 
-$(interface-library)-routines = $(interface-routines)
+$(interface-library)-routines = $(interface-routines) stack_chk_fail_local
 extra-libs += $(interface-library)
 extra-libs-others += $(interface-library)
 
diff --git a/mach/Versions b/mach/Versions
index 0097aad59b..b22fda4cf4 100644
--- a/mach/Versions
+++ b/mach/Versions
@@ -54,6 +54,10 @@  libc {
     # This was always there, but not exported as it should have been.
     mig_strncpy;
   }
+  GLIBC_2.4 {
+    # functions used by RPC stubs
+    __stack_chk_fail
+  }
   GLIBC_2.23 {
     __mach_host_self_;
   }
diff --git a/mach/stack_chk_fail_local.c b/mach/stack_chk_fail_local.c
new file mode 100644
index 0000000000..305871fbc0
--- /dev/null
+++ b/mach/stack_chk_fail_local.c
@@ -0,0 +1 @@ 
+#include <debug/stack_chk_fail_local.c>