Re: Copy relocations against versioned symbols with aliases without a default version

[Florian Weimer <fweimer at redhat dot com>]

On 03/07/2017 04:52 PM, H.J. Lu wrote:
> On Tue, Mar 7, 2017 at 6:57 AM, Florian Weimer <fweimer@redhat.com> wrote:
> > I'm trying to clean up the timezone code in glibc.  Historically, we export
> > both tzname and __tzname, and the symbols are aliased:
> >
> >    271: 0000000000399380    16 OBJECT  GLOBAL DEFAULT   33
> > __tzname@@GLIBC_2.2.5
> >    589: 0000000000399380    16 OBJECT  WEAK   DEFAULT   33
> > tzname@@GLIBC_2.2.5
> >
> > __tzname was exported more or less by accident, so I want to turn it into a
> > compat symbol, resulting in:
> >
> >    271: 0000000000399380    16 OBJECT  GLOBAL DEFAULT   33
> > __tzname@GLIBC_2.2.5
> >    589: 0000000000399380    16 OBJECT  WEAK   DEFAULT   33
> > tzname@@GLIBC_2.2.5
> >
> > When linking an object file with an R_X86_64_32S relocation against tzname
> >
> >   bb:   4d 8b 64 dd 00          mov    0x0(%r13,%rbx,8),%r12
> >   c0:   48 8b 2c dd 00 00 00    mov    0x0(,%rbx,8),%rbp
> >   c7:   00
> >                         c4: R_X86_64_32S        tzname
> >   c8:   4c 89 e6                mov    %r12,%rsi
> >
> > to this variant, I get this error:
> >
> > /usr/bin/ld: …/tst-timezone.o(.text+0xc4): unresolvable R_X86_64_32S
> > relocation against symbol `tzname@@GLIBC_2.2.5'
> >
> > This is really unexpected.  It seems that something causes the hidden bit to
> > be set on the compat symbol version for __tzname.  I assume ld treats this
> > as an indication that a hidden alias symbol exists, so that it cannot create
> > a copy relocation without breaking the alias.
> >
> > This is really fishy.  The hidden bit is documented as “If the highest bit
> > (bit 15) is set this is a hidden symbol which cannot be referenced from
> > outside the object.” in <https://www.akkadia.org/drepper/symbol-versioning>,
> > but this is not what is implemented because references to these
> > supposedly-hidden symbols work just fine if you specify an explicit symbol
> > version, both at static link time and at run time (which is not something we
> > can change in the dynamic linker because we have many existing binaries
> > which use hidden symbol versions for supposedly-exported non-default compat
> > symbols).  So there is no real reason why the static linker has to refuse to
> > create a copy relocation in such a cases.
> >
> > Comments?
>
> Do you have a standalone testcase?

Not really, I only have the attached glibc patch (against current 
master), which causes a link failure in timezone/tst-timezone.

Thanks,
Florian

Isolate global timezone variables in <time/time-variables.h>

This change makes it more explicit which subsystems access these
variables (which are thread safety hazards).

2017-03-07  Florian Weimer  <fweimer@redhat.com>

	Isolate global timezone variables in <time/time-variables.h>.
	* time/time-variables.h: New file.
	* time/time.h (__tzname, __daylight, __timezone): Remove private
	declarations.
	(tzname, daylight, timezone): Update comments.
	(__isleap): Remove definition.
	* include/time.h (__time_isleap): Declare.
	* time/Makefile (routines): Add isleap.
	(tests): Add tst-isleap.
	* time/Versions (GLIBC_2.26): Empty section, to enable
	SHLIB_COMPAT conditional on GLIBC_2_26.
	(GLIBC_PRIVATE): Export __time_isleap.
	* time/dysize.c (dysize): Call __time_isleap
	instead of __isleap.
	* time/getdate.c (check_mday): Likewise.
	* time/offtime.c (__offtime): Likewise.
	* time/gmtime.c: Include <time/time-variables.h>.
	* time/localtime.c: Likewise.
	* time/tzfile.c: Likewise.
	* time/tzset.c: Likewise.
	(__tzname, __daylight, __timezone): Turn into compat symbols.
	(compute_change): Call __time_isleap instead of __isleap.
	* time/strftime_l.c: Include <time/time-variables.h>.  Reorganize
	includes.
	(tzname): Declare for !_LIBC only.
	(__isleap): Define as __time_isleap.
	* time/isleap.c: New file.
	* time/tst-isleap.c: Likewise.
	* locale/programs/ld-time.c (time_finish): Call __time_isleap
	instead of __isleap.
	* sysdeps/posix/gettimeofday.c: Include <time/time-variables.h>.

diff --git a/NEWS b/NEWS
index d87e9ce..40ad8b2 100644
--- a/NEWS
+++ b/NEWS
@@ -11,6 +11,9 @@ Version 2.26
   transliteration tables are all updated to Unicode 9.0.0, using
   generator scripts contributed by Mike FABIAN (Red Hat).
 
+* The private identifiers __daylight, __isleap, __timezone, __tzname have
+  been removed from the <time.h> header file.
+
 Security related changes:
 
   [Add security related changes here]
diff --git a/include/time.h b/include/time.h
index 6badf0e..08454ef 100644
--- a/include/time.h
+++ b/include/time.h
@@ -3,6 +3,7 @@
 
 #ifndef _ISOMAC
 # include <xlocale.h>
+# include <stdbool.h>
 
 __BEGIN_DECLS
 
@@ -31,8 +32,8 @@ struct tm;
 /* Defined in mktime.c.  */
 extern const unsigned short int __mon_yday[2][13] attribute_hidden;
 
-/* Defined in localtime.c.  */
-extern struct tm _tmbuf attribute_hidden;
+bool __time_isleap (int year) internal_function;
+libc_hidden_proto (__time_isleap)
 
 /* Defined in tzset.c.  */
 extern char *__tzstring (const char *string);
diff --git a/locale/programs/ld-time.c b/locale/programs/ld-time.c
index 32e9c41..03c219b 100644
--- a/locale/programs/ld-time.c
+++ b/locale/programs/ld-time.c
@@ -26,6 +26,7 @@
 #include <wchar.h>
 #include <stdint.h>
 #include <sys/uio.h>
+#include <time.h>
 
 #include <assert.h>
 
@@ -353,7 +354,8 @@ No definition for %s category found"), "LC_TIME"));
 			   > days_per_month[time->era_entries[idx].start_date[1]])
 		       || (time->era_entries[idx].start_date[1] == 2
 			   && time->era_entries[idx].start_date[2] == 29
-			   && !__isleap (time->era_entries[idx].start_date[0])))
+			   && !__time_isleap (time->era_entries[idx]
+					      .start_date[0])))
 		      && !be_quiet)
 			  WITH_CUR_LOCALE (error (0, 0, _("\
 %s: starting date is invalid in string %Zd in `era' field"),
@@ -430,7 +432,8 @@ No definition for %s category found"), "LC_TIME"));
 			   > days_per_month[time->era_entries[idx].stop_date[1]])
 		       || (time->era_entries[idx].stop_date[1] == 2
 			   && time->era_entries[idx].stop_date[2] == 29
-			   && !__isleap (time->era_entries[idx].stop_date[0])))
+			   && !__time_isleap (time->era_entries[idx]
+					      .stop_date[0])))
 		      && !be_quiet)
 			  WITH_CUR_LOCALE (error (0, 0, _("\
 %s: invalid stopping date in string %Zd in `era' field"),
diff --git a/sysdeps/posix/gettimeofday.c b/sysdeps/posix/gettimeofday.c
index 41a8217..9ed4c60 100644
--- a/sysdeps/posix/gettimeofday.c
+++ b/sysdeps/posix/gettimeofday.c
@@ -19,6 +19,9 @@
 #include <time.h>
 #include <sys/time.h>
 
+/* For __tzname, __timezone, __daylight.  */
+#include <time/time-variables.h>
+
 /* Get the current time of day and timezone information,
    putting it into *TV and *TZ.  If TZ is NULL, *TZ is not filled.
    Returns 0 on success, -1 on errors.  */
diff --git a/time/Makefile b/time/Makefile
index 317c4d8..d21d90a 100644
--- a/time/Makefile
+++ b/time/Makefile
@@ -36,14 +36,14 @@ routines := offtime asctime clock ctime ctime_r difftime \
 	    stime dysize timegm ftime			 \
 	    getdate strptime strptime_l			 \
 	    strftime wcsftime strftime_l wcsftime_l	 \
-	    timespec_get
+	    timespec_get isleap
 aux :=	    era alt_digit lc-time-cleanup
 
 tests	:= test_time clocktest tst-posixtz tst-strptime tst_wcsftime \
 	   tst-getdate tst-mktime tst-mktime2 tst-ftime_l tst-strftime \
 	   tst-mktime3 tst-strptime2 bug-asctime bug-asctime_r bug-mktime1 \
 	   tst-strptime3 bug-getdate1 tst-strptime-whitespace tst-ftime \
-	   tst-tzname
+	   tst-tzname tst-isleap
 
 include ../Rules
 
diff --git a/time/Versions b/time/Versions
index fd83818..b7feea7 100644
--- a/time/Versions
+++ b/time/Versions
@@ -1,6 +1,6 @@
 libc {
   GLIBC_2.0 {
-    # global variables
+    # Global variables, exported as compat symbols.
     __daylight; __timezone; __tzname;
 
     # functions with special/multiple interfaces
@@ -65,4 +65,10 @@ libc {
   GLIBC_2.16 {
     timespec_get;
   }
+  GLIBC_2.26 {
+  }
+
+  GLIBC_PRIVATE {
+    __time_isleap;
+  }
 }
diff --git a/time/dysize.c b/time/dysize.c
index feed19c..ffc5dd3 100644
--- a/time/dysize.c
+++ b/time/dysize.c
@@ -20,5 +20,5 @@
 int
 dysize (int year)
 {
-  return __isleap (year) ? 366 : 365;
+  return __time_isleap (year) ? 366 : 365;
 }
diff --git a/time/getdate.c b/time/getdate.c
index 28ea482..bef9727 100644
--- a/time/getdate.c
+++ b/time/getdate.c
@@ -95,7 +95,7 @@ check_mday (int year, int mon, int mday)
 	return 1;
       break;
     case 1:
-      if (mday >= 1 && mday <= (__isleap (year) ? 29 : 28))
+      if (mday >= 1 && mday <= (__time_isleap (year) ? 29 : 28))
 	return 1;
       break;
     }
diff --git a/time/gmtime.c b/time/gmtime.c
index 049d551..1fc129b 100644
--- a/time/gmtime.c
+++ b/time/gmtime.c
@@ -17,6 +17,7 @@
    <http://www.gnu.org/licenses/>.  */
 
 #include <time.h>
+#include <time/time-variables.h>
 
 /* Return the `struct tm' representation of *T in UTC,
    using *TP to store the result.  */
diff --git a/time/isleap.c b/time/isleap.c
new file mode 100644
index 0000000..5f15709
--- /dev/null
+++ b/time/isleap.c
@@ -0,0 +1,27 @@
+/* Leap year detection.
+   Copyright (C) 2017 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/>.  */
+
+#include <time.h>
+
+bool
+internal_function
+__time_isleap (int year)
+{
+  return year % 4 == 0 && (year % 100 != 0 || year % 400 == 0);
+}
+libc_hidden_def (__time_isleap)
diff --git a/time/localtime.c b/time/localtime.c
index 07dd67c..3a38cde 100644
--- a/time/localtime.c
+++ b/time/localtime.c
@@ -17,6 +17,7 @@
    <http://www.gnu.org/licenses/>.  */
 
 #include <time.h>
+#include <time/time-variables.h>
 
 /* The C Standard says that localtime and gmtime return the same pointer.  */
 struct tm _tmbuf;
diff --git a/time/offtime.c b/time/offtime.c
index 75a28fe..1729a96 100644
--- a/time/offtime.c
+++ b/time/offtime.c
@@ -57,7 +57,7 @@ __offtime (const time_t *t, long int offset, struct tm *tp)
 #define DIV(a, b) ((a) / (b) - ((a) % (b) < 0))
 #define LEAPS_THRU_END_OF(y) (DIV (y, 4) - DIV (y, 100) + DIV (y, 400))
 
-  while (days < 0 || days >= (__isleap (y) ? 366 : 365))
+  while (days < 0 || days >= (__time_isleap (y) ? 366 : 365))
     {
       /* Guess a corrected year, assuming 365 days per year.  */
       time_t yg = y + days / 365 - (days % 365 < 0);
@@ -76,7 +76,7 @@ __offtime (const time_t *t, long int offset, struct tm *tp)
       return 0;
     }
   tp->tm_yday = days;
-  ip = __mon_yday[__isleap(y)];
+  ip = __mon_yday[__time_isleap (y)];
   for (y = 11; days < (long int) ip[y]; --y)
     continue;
   days -= ip[y];
diff --git a/time/strftime_l.c b/time/strftime_l.c
index eb3efb8..2a13af5 100644
--- a/time/strftime_l.c
+++ b/time/strftime_l.c
@@ -33,28 +33,30 @@
 # define MULTIBYTE_IS_FORMAT_SAFE 1
 # define STDC_HEADERS 1
 # include "../locale/localeinfo.h"
-#endif
 
-#if defined emacs && !defined HAVE_BCOPY
-# define HAVE_MEMCPY 1
+# define __isleap(year) __time_isleap (year)
+# include <time.h>
+# include <time/time-variables.h>
 #endif
 
 #include <ctype.h>
 #include <sys/types.h>		/* Some systems define `time_t' here.  */
 
-#ifdef TIME_WITH_SYS_TIME
-# include <sys/time.h>
-# include <time.h>
-#else
-# ifdef HAVE_SYS_TIME_H
+#ifndef _LIBC
+# ifdef TIME_WITH_SYS_TIME
 #  include <sys/time.h>
-# else
 #  include <time.h>
+# else
+#  ifdef HAVE_SYS_TIME_H
+#   include <sys/time.h>
+#  else
+#   include <time.h>
+#  endif
 # endif
-#endif
-#if HAVE_TZNAME
+# if HAVE_TZNAME
 extern char *tzname[];
-#endif
+# endif
+#endif /* !_LIBC */
 
 /* Do multibyte processing if multibytes are supported, unless
    multibyte sequences are safe in formats.  Multibyte sequences are
diff --git a/time/time-variables.h b/time/time-variables.h
new file mode 100644
index 0000000..0ca9970
--- /dev/null
+++ b/time/time-variables.h
@@ -0,0 +1,44 @@
+/* Global variables used by the time subsystem.
+   Copyright (C) 2017 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/>.  */
+
+/* This header should only be used within the time subsystem, but
+   there are some historic external references.  The global variables
+   declared here cannot be accessed in a thread-safe manner.  */
+
+#ifndef TIME_VARIABLES_H
+#define TIME_VARIABLES_H
+
+#include <stdbool.h>
+#include <time.h>
+
+/* Global variables updated by tzset, localtime.  These are aliased to
+   their counterparts without the __ prefix.
+
+   Ideally, we should turn __tzname, __daylight, __timezone into
+   compat symbols.  However, ld cannot process certain relocations for
+   symbols such as tzname, which have an alias which is a compat
+   symbol.  The alias is considered hidden, and ld rejects the link
+   because it assumes that the alias relationship is broken.  */
+extern char *__tzname[2];
+extern int __daylight;
+extern long int __timezone;
+
+/* Defined in localtime.c.  */
+extern struct tm _tmbuf attribute_hidden;
+
+#endif /* TIME_VARIABLES_H */
diff --git a/time/time.h b/time/time.h
index 7f98338..9383c09 100644
--- a/time/time.h
+++ b/time/time.h
@@ -160,16 +160,8 @@ extern char *ctime_r (const time_t *__restrict __timer,
 		      char *__restrict __buf) __THROW;
 #endif	/* POSIX */
 
-
-/* Defined in localtime.c.  */
-extern char *__tzname[2];	/* Current timezone names.  */
-extern int __daylight;		/* If daylight-saving time is ever in use.  */
-extern long int __timezone;	/* Seconds west of UTC.  */
-
-
 #ifdef	__USE_POSIX
-/* Same as above.  */
-extern char *tzname[2];
+extern char *tzname[2];   /* Current timezone names.  */
 
 /* Set time conversion information from the TZ environment variable.
    If TZ is not defined, a locale-dependent default is used.  */
@@ -177,8 +169,8 @@ extern void tzset (void) __THROW;
 #endif
 
 #if defined __USE_MISC || defined __USE_XOPEN
-extern int daylight;
-extern long int timezone;
+extern int daylight;        /* If daylight-saving time is ever in use.  */
+extern long int timezone;   /* Seconds west of UTC.  */
 #endif
 
 #ifdef __USE_MISC
@@ -187,13 +179,6 @@ extern long int timezone;
 extern int stime (const time_t *__when) __THROW;
 #endif
 
-
-/* Nonzero if YEAR is a leap year (every 4 years,
-   except every 100th isn't, and every 400th is).  */
-#define __isleap(year)	\
-  ((year) % 4 == 0 && ((year) % 100 != 0 || (year) % 400 == 0))
-
-
 #ifdef __USE_MISC
 /* Miscellaneous functions many Unices inherited from the public domain
    localtime package.  These are included only for compatibility.  */
diff --git a/time/tst-isleap.c b/time/tst-isleap.c
new file mode 100644
index 0000000..590a403
--- /dev/null
+++ b/time/tst-isleap.c
@@ -0,0 +1,40 @@
+/* Test leap year processing.
+   Copyright (C) 2017 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/>.  */
+
+#include <time.h>
+#include <support/check.h>
+
+static int
+do_test (void)
+{
+  TEST_VERIFY (!__time_isleap (-100));
+  TEST_VERIFY (__time_isleap (0));
+  TEST_VERIFY (!__time_isleap (100));
+  TEST_VERIFY (!__time_isleap (200));
+  TEST_VERIFY (!__time_isleap (300));
+  TEST_VERIFY (__time_isleap (400));
+  TEST_VERIFY (!__time_isleap (1900));
+  TEST_VERIFY (__time_isleap (1996));
+  TEST_VERIFY (__time_isleap (2000));
+  TEST_VERIFY (__time_isleap (2004));
+  TEST_VERIFY (__time_isleap (2008));
+  TEST_VERIFY (!__time_isleap (2100));
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/time/tzfile.c b/time/tzfile.c
index d412469..9290dc6 100644
--- a/time/tzfile.c
+++ b/time/tzfile.c
@@ -26,6 +26,7 @@
 #include <sys/stat.h>
 #include <stdint.h>
 
+#include <time/time-variables.h>
 #include <timezone/tzfile.h>
 
 int __use_tzfile;
diff --git a/time/tzset.c b/time/tzset.c
index 8868e9a..1cef415 100644
--- a/time/tzset.c
+++ b/time/tzset.c
@@ -24,13 +24,22 @@
 #include <stdlib.h>
 #include <string.h>
 #include <time.h>
+#include <libc-symbols.h>
+#include <shlib-compat.h>
 
+#include <time/time-variables.h>
 #include <timezone/tzfile.h>
 
 char *__tzname[2] = { (char *) "GMT", (char *) "GMT" };
 int __daylight = 0;
 long int __timezone = 0L;
 
+#if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_26)
+compat_symbol (libc, __tzname, __tzname, GLIBC_2_0);
+compat_symbol (libc, __daylight, __daylight, GLIBC_2_0);
+compat_symbol (libc, __timezone, __timezone, GLIBC_2_0);
+#endif
+
 weak_alias (__tzname, tzname)
 weak_alias (__daylight, daylight)
 weak_alias (__timezone, timezone)
@@ -457,7 +466,7 @@ compute_change (tz_rule *rule, int year)
 	 add SECSPERDAY times the day number-1 to the time of
 	 January 1, midnight, to get the day.  */
       t += (rule->d - 1) * SECSPERDAY;
-      if (rule->d >= 60 && __isleap (year))
+      if (rule->d >= 60 && __time_isleap (year))
 	t += SECSPERDAY;
       break;
 
@@ -473,7 +482,7 @@ compute_change (tz_rule *rule, int year)
 	unsigned int i;
 	int d, m1, yy0, yy1, yy2, dow;
 	const unsigned short int *myday =
-	  &__mon_yday[__isleap (year)][rule->m];
+	  &__mon_yday[__time_isleap (year)][rule->m];
 
 	/* First add SECSPERDAY for each day in months before M.  */
 	t += myday[-1] * SECSPERDAY;