diff mbox

Avoid .symver on common symbols [BZ #21666]

Message ID CAMe9rOryBUqXQgYU=0mzD=6GAvr=zJHLwDfd3XqaFpS8KL0CUw@mail.gmail.com
State New, archived
Headers

Commit Message

H.J. Lu June 23, 2017, 4:26 p.m. UTC 
  On Fri, Jun 23, 2017 at 9:17 AM, Florian Weimer <fweimer@redhat.com> wrote:
> On 06/23/2017 06:11 PM, H.J. Lu wrote:
>> +/* Define the variables used for the interface.  Avoid .symver on common
>> +   symbol, which just creates a new common symbol, not an alias.  */
>> +char *loc1 = NULL;
>> +char *loc2 = NULL;
>
> I think __attribute__ ((nocommon)) without the initializer would be more
> explicit.  We already use that for _res in resolv/res_libc.c.

Done.

> Does this result in a visible difference for applications?  If yes,
> please file a bug for this and reference it in the ChangeLog and commit
> message.

It will be very hard to tell since these symbols were exported from libc.so
by accident and we only keep them in libc.so for backward binary compatibility.
Application can no longer reference them from libc.so.

> Why didn't our test suite catch it?

We never tried to catch errors like this.

Here is the updated patch.  OK for master?

Thanks.

Comments

Florian Weimer June 23, 2017, 9:13 p.m. UTC | #1 
On 06/23/2017 06:26 PM, H.J. Lu wrote:

>> Does this result in a visible difference for applications?  If yes,
>> please file a bug for this and reference it in the ChangeLog and commit
>> message.
> 
> It will be very hard to tell since these symbols were exported from libc.so
> by accident and we only keep them in libc.so for backward binary compatibility.
> Application can no longer reference them from libc.so.

Ah, it's the old <regexp.h> interface, and the symbols were exported
deliberately.  We apparently do not have any tests for it.

> Here is the updated patch.  OK for master?

Yes, please.  Thanks.

Florian
Szabolcs Nagy July 12, 2017, 4:56 p.m. UTC | #2 
On 23/06/17 22:13, Florian Weimer wrote:
> On 06/23/2017 06:26 PM, H.J. Lu wrote:
> 
>>> Does this result in a visible difference for applications?  If yes,
>>> please file a bug for this and reference it in the ChangeLog and commit
>>> message.
>>
>> It will be very hard to tell since these symbols were exported from libc.so
>> by accident and we only keep them in libc.so for backward binary compatibility.
>> Application can no longer reference them from libc.so.
> 
> Ah, it's the old <regexp.h> interface, and the symbols were exported
> deliberately.  We apparently do not have any tests for it.
> 
>> Here is the updated patch.  OK for master?
> 
> Yes, please.  Thanks.
> 

i think this should be ok for backporting too
since without it old releases cannot be built
with new binutils.
H.J. Lu July 12, 2017, 5:10 p.m. UTC | #3 
On Wed, Jul 12, 2017 at 9:56 AM, Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
> On 23/06/17 22:13, Florian Weimer wrote:
>> On 06/23/2017 06:26 PM, H.J. Lu wrote:
>>
>>>> Does this result in a visible difference for applications?  If yes,
>>>> please file a bug for this and reference it in the ChangeLog and commit
>>>> message.
>>>
>>> It will be very hard to tell since these symbols were exported from libc.so
>>> by accident and we only keep them in libc.so for backward binary compatibility.
>>> Application can no longer reference them from libc.so.
>>
>> Ah, it's the old <regexp.h> interface, and the symbols were exported
>> deliberately.  We apparently do not have any tests for it.
>>
>>> Here is the updated patch.  OK for master?
>>
>> Yes, please.  Thanks.
>>
>
> i think this should be ok for backporting too
> since without it old releases cannot be built
> with new binutils.
>

I have ported it to hjl/pr21120/2.25 branch.  I also backported 2 patches for
GCC 7. See:

https://sourceware.org/git/?p=glibc.git;a=shortlog;h=refs/heads/hjl/pr21120/2.25
Zack Weinberg July 12, 2017, 7:19 p.m. UTC | #4 
On Fri, Jun 23, 2017 at 5:13 PM, Florian Weimer <fweimer@redhat.com> wrote:
> On 06/23/2017 06:26 PM, H.J. Lu wrote:
>
>>> Does this result in a visible difference for applications?  If yes,
>>> please file a bug for this and reference it in the ChangeLog and commit
>>> message.
>>
>> It will be very hard to tell since these symbols were exported from libc.so
>> by accident and we only keep them in libc.so for backward binary compatibility.
>> Application can no longer reference them from libc.so.
>
> Ah, it's the old <regexp.h> interface, and the symbols were exported
> deliberately.  We apparently do not have any tests for it.

FYI, there's some reason to believe that this interface never actually
worked correctly -- see the discussion from when it was deprecated.

zw
diff mbox

Patch

From 67145d84d9719d270f63f4399e337a4bdcb778f5 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Fri, 23 Jun 2017 09:03:19 -0700
Subject: [PATCH] Avoid .symver on common symbols [BZ #21666]

The .symver directive on common symbol just creates a new common symbol,
not an alias and the newer assembler with the bug fix for

https://sourceware.org/bugzilla/show_bug.cgi?id=21661

will issue an error.  Before the fix, we got

$ readelf -sW libc.so | grep "loc[12s]"
  5109: 00000000003a0608     8 OBJECT  LOCAL  DEFAULT   36 loc1
  5188: 00000000003a0610     8 OBJECT  LOCAL  DEFAULT   36 loc2
  5455: 00000000003a0618     8 OBJECT  LOCAL  DEFAULT   36 locs
  6575: 00000000003a05f0     8 OBJECT  GLOBAL DEFAULT   36 locs@GLIBC_2.2.5
  7156: 00000000003a05f8     8 OBJECT  GLOBAL DEFAULT   36 loc1@GLIBC_2.2.5
  7312: 00000000003a0600     8 OBJECT  GLOBAL DEFAULT   36 loc2@GLIBC_2.2.5

in libc.so.  The versioned loc1, loc2 and locs have the wrong addresses.
After the fix, we got

$ readelf -sW libc.so | grep "loc[12s]"
  6570: 000000000039e3b8     8 OBJECT  GLOBAL DEFAULT   34 locs@GLIBC_2.2.5
  7151: 000000000039e3c8     8 OBJECT  GLOBAL DEFAULT   34 loc1@GLIBC_2.2.5
  7307: 000000000039e3c0     8 OBJECT  GLOBAL DEFAULT   34 loc2@GLIBC_2.2.5

	[BZ #21666]
	* misc/regexp.c (loc1): Add __attribute__ ((nocommon));
	(loc2): Likewise.
	(locs): Likewise.
---
 misc/regexp.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/misc/regexp.c b/misc/regexp.c
index 19d76c0..eaea7c3 100644
--- a/misc/regexp.c
+++ b/misc/regexp.c
@@ -29,14 +29,15 @@ 
 
 #if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_23)
 
-/* Define the variables used for the interface.  */
-char *loc1;
-char *loc2;
+/* Define the variables used for the interface.  Avoid .symver on common
+   symbol, which just creates a new common symbol, not an alias.  */
+char *loc1 __attribute__ ((nocommon));
+char *loc2 __attribute__ ((nocommon));
 compat_symbol (libc, loc1, loc1, GLIBC_2_0);
 compat_symbol (libc, loc2, loc2, GLIBC_2_0);
 
 /* Although we do not support the use we define this variable as well.  */
-char *locs;
+char *locs __attribute__ ((nocommon));
 compat_symbol (libc, locs, locs, GLIBC_2_0);
 
 
-- 
2.9.4