scripts/test-installation.pl: Handle NSS crypto libraries [BZ #21940]

Message ID 20170810123022.6DB7B439942E1@oldenburg.str.redhat.com
State New
Headers show

Commit Message

Florian Weimer Aug. 10, 2017, 12:30 p.m.
The warning looked like this:

Use of uninitialized value in string ne at
  …/scripts/test-installation.pl line 184, <LDD> line 24.

It is triggered by this line of ldd output:

 libfreebl3.so => /lib64/libfreebl3.so (0x00007f055003c000)

The other lines have a version in the soname:

 libanl.so.1 => /lib64/libanl.so.1 (0x00007f055023f000)

2017-08-10  Florian Weimer  <fweimer@redhat.com>

	[BZ #21940]
	* scripts/test-installation.pl: Handle NSS crypto libaries in ldd
	output.

Comments

Florian Weimer Oct. 5, 2017, 10:53 a.m. | #1
On 08/10/2017 02:30 PM, Florian Weimer wrote:
> The warning looked like this:
> 
> Use of uninitialized value in string ne at
>    …/scripts/test-installation.pl line 184, <LDD> line 24.
> 
> It is triggered by this line of ldd output:
> 
>   libfreebl3.so => /lib64/libfreebl3.so (0x00007f055003c000)
> 
> The other lines have a version in the soname:
> 
>   libanl.so.1 => /lib64/libanl.so.1 (0x00007f055023f000)
> 
> 2017-08-10  Florian Weimer<fweimer@redhat.com>
> 
> 	[BZ #21940]
> 	* scripts/test-installation.pl: Handle NSS crypto libaries in ldd
> 	output.

Ping?

<https://sourceware.org/ml/libc-alpha/2017-08/msg00391.html>

Thanks,
Florian
Rical Jasan Oct. 7, 2017, 10:50 a.m. | #2
On 08/10/2017 05:30 AM, Florian Weimer wrote:
> The warning looked like this:
> 
> Use of uninitialized value in string ne at
>   …/scripts/test-installation.pl line 184, <LDD> line 24.
> 
> It is triggered by this line of ldd output:
> 
>  libfreebl3.so => /lib64/libfreebl3.so (0x00007f055003c000)
> 
> The other lines have a version in the soname:
> 
>  libanl.so.1 => /lib64/libanl.so.1 (0x00007f055023f000)
> 
> 2017-08-10  Florian Weimer  <fweimer@redhat.com>
> 
> 	[BZ #21940]
> 	* scripts/test-installation.pl: Handle NSS crypto libaries in ldd
> 	output.
> 
> diff --git a/scripts/test-installation.pl b/scripts/test-installation.pl
> index 4b0e9f3c4a..466c526cc9 100755
> --- a/scripts/test-installation.pl
> +++ b/scripts/test-installation.pl
> @@ -177,10 +177,15 @@ open LDD, "ldd /tmp/test-prg$$ |"
>    or die ("Couldn't execute ldd");
>  while (<LDD>) {
>    if (/^\s*lib/) {
> +    # When libcrypt is linked against NSS, some of the referenced
> +    # libraries do not have a trailing version in their soname.
>      ($name, $version1, $version2) =
> -      /^\s*lib(\w*)\.so\.([0-9\.]*)\s*=>.*\.so\.([0-9\.]*)/;
> +      /^\s*lib(\w*)\.so(?:\.([0-9\.]*))?\s*=>.*\.so(?:\.([0-9\.]*))?/;

I would use:

/^\s*lib(\w+)\.so(?:\.([0-9\.]+))?\s*=>.*\.so(?:\.([0-9\.]+))?/
           ^                  ^                           ^
to avoid matching "lib.so.".

>      $found{$name} = 1;
> -    if ($versions{$name} ne $version1 || $version1 ne $version2) {
> +    if (defined($version1) != defined($version2)
> +	|| defined($version1) != defined($versions{$name})
> +	|| (defined($versions{$name})
> +	    && ($versions{$name} ne $version1 || $version1 ne $version2))) {
>        print "Library lib$name is not correctly installed.\n";
>        print "Please check your installation!\n";
>        print "Offending line of ldd output: $_\n";

It might help readability to follow up the match with something like:

 next if ! (defined($name) && defined($version1) && defined($version2));
 next if ! (exists $versions{$name}) && defined($versions{$name}));

(I seem to recall tests on hash entries creating them when they didn't
previously exist, but that may not matter here.  Something other than
`next' may also be desirable, and the opportunity for more fine-grained
error messages is introduced, if useful.)

Then you could retain the simpler conditional:

 if ($versions{$name} ne $version1 || $version1 ne $version2) {

That also continues using string comparisons, which would be better in
case there are multiple "."'s.  NSS installs *.so files on my system,
but I do have programs that link against libraries with multiple version
points; e.g.:

$ ldd /usr/bin/avprobe | grep bz2
	libbz2.so.1.0 => /usr/lib/libbz2.so.1.0 (0x00007f869fab8000)

Rical
Florian Weimer Oct. 10, 2017, 9:26 a.m. | #3
On 10/07/2017 12:50 PM, Rical Jasan wrote:

> I would use:
> 
> /^\s*lib(\w+)\.so(?:\.([0-9\.]+))?\s*=>.*\.so(?:\.([0-9\.]+))?/
>             ^                  ^                           ^
> to avoid matching "lib.so.".

I made the change in the attached patch.

>>       $found{$name} = 1;
>> -    if ($versions{$name} ne $version1 || $version1 ne $version2) {
>> +    if (defined($version1) != defined($version2)
>> +	|| defined($version1) != defined($versions{$name})
>> +	|| (defined($versions{$name})
>> +	    && ($versions{$name} ne $version1 || $version1 ne $version2))) {
>>         print "Library lib$name is not correctly installed.\n";
>>         print "Please check your installation!\n";
>>         print "Offending line of ldd output: $_\n";
> 
> It might help readability to follow up the match with something like:
> 
>   next if ! (defined($name) && defined($version1) && defined($version2));
>   next if ! (exists $versions{$name}) && defined($versions{$name}));

Is this really clearer?  What I wanted to express is “error if the 
defined-ness state is different across the three value, and if 
$versions{$name} is defined, it must much both versions”.  The original 
condition captures this fairly succinctly.

> (I seem to recall tests on hash entries creating them when they didn't
> previously exist, but that may not matter here.  Something other than
> `next' may also be desirable, and the opportunity for more fine-grained
> error messages is introduced, if useful.)

I suspect the original script was written for Perl 4, which did not have 
the exists operator.  I instinctively stuck to that baseline.

Anyway, I came up with something simpler, which sidesteps these issues.

> Then you could retain the simpler conditional:
> 
>   if ($versions{$name} ne $version1 || $version1 ne $version2) {
> 
> That also continues using string comparisons, which would be better in
> case there are multiple "."'s.

I only used != as the Boolean XOR operator on the numeric result from 
the defined operator.

Thanks,
Florian
The warning looked like this:

Use of uninitialized value in string ne at
  …/scripts/test-installation.pl line 184, <LDD> line 24.

It is triggered by this line of ldd output:

 libfreebl3.so => /lib64/libfreebl3.so (0x00007f055003c000)

The other lines have a version in the soname:

 libanl.so.1 => /lib64/libanl.so.1 (0x00007f055023f000)

2017-10-10  Florian Weimer  <fweimer@redhat.com>

	[BZ #21940]
	* scripts/test-installation.pl: Handle NSS crypto libaries in ldd
	output.

diff --git a/scripts/test-installation.pl b/scripts/test-installation.pl
index 74d25e1c8d..7eaeb1ae06 100755
--- a/scripts/test-installation.pl
+++ b/scripts/test-installation.pl
@@ -176,10 +176,17 @@ open LDD, "ldd /tmp/test-prg$$ |"
   or die ("Couldn't execute ldd");
 while (<LDD>) {
   if (/^\s*lib/) {
-    ($name, $version1, $version2) =
-      /^\s*lib(\w*)\.so\.([0-9\.]*)\s*=>.*\.so\.([0-9\.]*)/;
+    # When libcrypt is linked against NSS, some of the referenced
+    # libraries do not have a trailing version in their soname.
+    my ($name, $version1, $version2) =
+      /^\s*lib(\w+)\.so(?:\.([0-9\.]+))?\s*=>.*\.so(?:\.([0-9\.]+))?/;
     $found{$name} = 1;
-    if ($versions{$name} ne $version1 || $version1 ne $version2) {
+    # Version strings are either undefined or non-empty.
+    $version1 = '' unless defined $version1;
+    $version2 = '' unless defined $version2;
+    my $vername = $versions{$name};
+    $vername = '' unless defined $vername;
+    if ($version1 ne $version2 || $vername ne $version1) {
       print "Library lib$name is not correctly installed.\n";
       print "Please check your installation!\n";
       print "Offending line of ldd output: $_\n";
Rical Jasan Oct. 18, 2017, 5:06 a.m. | #4
On 10/10/2017 02:26 AM, Florian Weimer wrote:
> On 10/07/2017 12:50 PM, Rical Jasan wrote:
> 
>> I would use:
>>
>> /^\s*lib(\w+)\.so(?:\.([0-9\.]+))?\s*=>.*\.so(?:\.([0-9\.]+))?/
>>             ^                  ^                           ^
>> to avoid matching "lib.so.".
> 
> I made the change in the attached patch.
> 
>>>       $found{$name} = 1;
>>> -    if ($versions{$name} ne $version1 || $version1 ne $version2) {
>>> +    if (defined($version1) != defined($version2)
>>> +    || defined($version1) != defined($versions{$name})
>>> +    || (defined($versions{$name})
>>> +        && ($versions{$name} ne $version1 || $version1 ne $version2))) {
>>>
>>>         print "Library lib$name is not correctly installed.\n";
>>>         print "Please check your installation!\n";
>>>         print "Offending line of ldd output: $_\n";
>>
>> It might help readability to follow up the match with something like:
>>
>>   next if ! (defined($name) && defined($version1) && defined($version2));
>>   next if ! (exists $versions{$name}) && defined($versions{$name}));
> 
> Is this really clearer?  What I wanted to express is “error if the
> defined-ness state is different across the three value, and if
> $versions{$name} is defined, it must much both versions”.  The original
> condition captures this fairly succinctly.

Converting to a natural language expression was exactly the trouble I
had.  I've attached an alternative proposal.  I'm not sure if the
"Unexpected dependency" message is correct, but the
existence/defined-ness check on $versions{$name} seems the important
bit, which makes the subsequent transitive version tests safe.

My system has unversioned NSS libraries, but I don't have a sandbox
handy to run a `make install' (which is when it looks like this script
is run), so it's untested.

>> (I seem to recall tests on hash entries creating them when they didn't
>> previously exist, but that may not matter here.  Something other than
>> `next' may also be desirable, and the opportunity for more fine-grained
>> error messages is introduced, if useful.)
> 
> I suspect the original script was written for Perl 4, which did not have
> the exists operator.  I instinctively stuck to that baseline.

This script definitely wouldn't pass a compilation check using the
strict pragma; I noticed you slipped in a `my', but the whole thing
suffers from the same scoping issue.  I see install.texi says Perl 5 is
recommended, and I noticed perl(1) says:

  Using the "use strict" pragma ensures that all variables are properly
  declared and prevents other misuses of legacy Perl features.

I can update the script (and others) to specifically address such
"miuses" in a separate patchset, if it's considered worthwhile.


Rical

> Anyway, I came up with something simpler, which sidesteps these issues.
> 
>> Then you could retain the simpler conditional:
>>
>>   if ($versions{$name} ne $version1 || $version1 ne $version2) {
>>
>> That also continues using string comparisons, which would be better in
>> case there are multiple "."'s.
> 
> I only used != as the Boolean XOR operator on the numeric result from
> the defined operator.
> 
> Thanks,
> Florian
> 
> bug21940.patch
> 
> 
> The warning looked like this:
> 
> Use of uninitialized value in string ne at
>   …/scripts/test-installation.pl line 184, <LDD> line 24.
> 
> It is triggered by this line of ldd output:
> 
>  libfreebl3.so => /lib64/libfreebl3.so (0x00007f055003c000)
> 
> The other lines have a version in the soname:
> 
>  libanl.so.1 => /lib64/libanl.so.1 (0x00007f055023f000)
> 
> 2017-10-10  Florian Weimer  <fweimer@redhat.com>
> 
> 	[BZ #21940]
> 	* scripts/test-installation.pl: Handle NSS crypto libaries in ldd
> 	output.
> 
> diff --git a/scripts/test-installation.pl b/scripts/test-installation.pl
> index 74d25e1c8d..7eaeb1ae06 100755
> --- a/scripts/test-installation.pl
> +++ b/scripts/test-installation.pl
> @@ -176,10 +176,17 @@ open LDD, "ldd /tmp/test-prg$$ |"
>    or die ("Couldn't execute ldd");
>  while (<LDD>) {
>    if (/^\s*lib/) {
> -    ($name, $version1, $version2) =
> -      /^\s*lib(\w*)\.so\.([0-9\.]*)\s*=>.*\.so\.([0-9\.]*)/;
> +    # When libcrypt is linked against NSS, some of the referenced
> +    # libraries do not have a trailing version in their soname.
> +    my ($name, $version1, $version2) =
> +      /^\s*lib(\w+)\.so(?:\.([0-9\.]+))?\s*=>.*\.so(?:\.([0-9\.]+))?/;
>      $found{$name} = 1;
> -    if ($versions{$name} ne $version1 || $version1 ne $version2) {
> +    # Version strings are either undefined or non-empty.
> +    $version1 = '' unless defined $version1;
> +    $version2 = '' unless defined $version2;
> +    my $vername = $versions{$name};
> +    $vername = '' unless defined $vername;
> +    if ($version1 ne $version2 || $vername ne $version1) {
>        print "Library lib$name is not correctly installed.\n";
>        print "Please check your installation!\n";
>        print "Offending line of ldd output: $_\n";
diff --git a/scripts/test-installation.pl b/scripts/test-installation.pl
index 45c666b0a2..fcb7707889 100755
--- a/scripts/test-installation.pl
+++ b/scripts/test-installation.pl
@@ -171,14 +171,20 @@ if ($?) {
 
 $ok = 1;
 %found = ();
+$librgx = qr/lib(\w+)\.so(?:\.([0-9\.]+))?/;
 
 open LDD, "ldd /tmp/test-prg$$ |"
   or die ("Couldn't execute ldd");
 while (<LDD>) {
-  if (/^\s*lib/) {
+  if (m,^\s*${librgx}\s*=>\s*(?:/[^/]+)*/${librgx},) {
     ($name, $version1, $version2) =
-      /^\s*lib(\w*)\.so\.([0-9\.]*)\s*=>.*\.so\.([0-9\.]*)/;
+      ($1, defined($2) ? $2 : '', defined($4) ? $4 : '');
     $found{$name} = 1;
+    if (!exists $versions{$name}) {
+      print "Unexpected dependency: lib$name\n";
+      $ok = 0;
+      next;
+    }
     if ($versions{$name} ne $version1 || $version1 ne $version2) {
       print "Library lib$name is not correctly installed.\n";
       print "Please check your installation!\n";
Florian Weimer Dec. 14, 2017, 3:10 p.m. | #5
On 10/18/2017 07:06 AM, Rical Jasan wrote:
> diff --git a/scripts/test-installation.pl b/scripts/test-installation.pl
> index 45c666b0a2..fcb7707889 100755
> --- a/scripts/test-installation.pl
> +++ b/scripts/test-installation.pl
> @@ -171,14 +171,20 @@ if ($?) {
>   
>   $ok = 1;
>   %found = ();
> +$librgx = qr/lib(\w+)\.so(?:\.([0-9\.]+))?/;
>   
>   open LDD, "ldd /tmp/test-prg$$ |"
>     or die ("Couldn't execute ldd");
>   while (<LDD>) {
> -  if (/^\s*lib/) {
> +  if (m,^\s*${librgx}\s*=>\s*(?:/[^/]+)*/${librgx},) {
>       ($name, $version1, $version2) =
> -      /^\s*lib(\w*)\.so\.([0-9\.]*)\s*=>.*\.so\.([0-9\.]*)/;
> +      ($1, defined($2) ? $2 : '', defined($4) ? $4 : '');
>       $found{$name} = 1;
> +    if (!exists $versions{$name}) {
> +      print "Unexpected dependency: lib$name\n";
> +      $ok = 0;
> +      next;
> +    }

This looks fine to me.  Could you commit it with a ChangeLog entry?

Thanks,
Florian
Rical Jasan Dec. 19, 2017, 9:16 a.m. | #6
On 12/14/2017 07:10 AM, Florian Weimer wrote:
> On 10/18/2017 07:06 AM, Rical Jasan wrote:
>> diff --git a/scripts/test-installation.pl b/scripts/test-installation.pl
>> index 45c666b0a2..fcb7707889 100755
>> --- a/scripts/test-installation.pl
>> +++ b/scripts/test-installation.pl
>> @@ -171,14 +171,20 @@ if ($?) {
>>     $ok = 1;
>>   %found = ();
>> +$librgx = qr/lib(\w+)\.so(?:\.([0-9\.]+))?/;
>>     open LDD, "ldd /tmp/test-prg$$ |"
>>     or die ("Couldn't execute ldd");
>>   while (<LDD>) {
>> -  if (/^\s*lib/) {
>> +  if (m,^\s*${librgx}\s*=>\s*(?:/[^/]+)*/${librgx},) {
>>       ($name, $version1, $version2) =
>> -      /^\s*lib(\w*)\.so\.([0-9\.]*)\s*=>.*\.so\.([0-9\.]*)/;
>> +      ($1, defined($2) ? $2 : '', defined($4) ? $4 : '');
>>       $found{$name} = 1;
>> +    if (!exists $versions{$name}) {
>> +      print "Unexpected dependency: lib$name\n";
>> +      $ok = 0;
>> +      next;
>> +    }
> 
> This looks fine to me.  Could you commit it with a ChangeLog entry?

I have the resources available to set up burner systems now, so after
reacquainting myself with this patch, I set about testing it.  Not only
is test-installation.pl the very last thing run during `make install',
but it _only_ happens when prefix=/usr and DESTDIR is not set, so even
testing the patch requires bombing the system libc.

The above tidbit actually makes the situation worse because instead of
having some "uninitialized value" warnings printed to stderr by Perl
with an otherwise successful installation, that "Unexpected dependency"
check will cause `make install' to fail, at the very end, after
everything has already been installed.

What needs to happen is to make libfreebl3 an expected dependency, as
well as allowing for unversioned sonames.  I've attached a patch that
works for me, but it definitely stems from hacking just enough to make
it work.  There are some design choices to be made, so others need to
weigh in on whether they think what I did is appropriate.

The process starts with shlib-versions, which defines valid versions of
shared libraries, both installed by glibc as well as external
dependencies.  Note that in this file, versions are not prefixed by a
dot (important later).  The choice I made here was to simply not define
a version.

scripts/soversions.awk uses shlib-versions.v to create soversions.i in
the build root.  I'm not sure where shlib-versions.v comes from, but it
is a generated file as well.  I chose to tag the special case of no
version with an arbitrary version of "undef" because if it is left blank
at this point, the whitespace-delimited fields in soversions.i will be
parsed incorrectly when generating soversions.mk in Makeconfig.

Correspondingly, Makeconfig is given a special case for the "undef"
version, which generates soversions.mk lines absent of any versioning.
There is some interesting voodoo here with what appears to be a
superfluous dot preceding the version, but I tried stripping it to
condense things and wound up with a broken installation with programs
that couldn't find "libc.so6" at runtime, so I was convinced I should
keep my unversioned soname propagation as an independent, exceptional case.

Lastly, scripts/test-installation.pl populates its %versions hash by
matching lines in soversions.mk with versions prefixed with a dot, so I
made the dot optional.  This results in an empty string for the version
of unversioned libraries, but that is specific to the library name, and
should be OK.  This is where we meet the above diff, which will match an
unversioned library, so those empty strings will result in equality for
a given library, and since an entry was defined for the library in the
%versions hash, it isn't an unexpected dependency either.

There is some ordering in soversions.awk that I don't quite understand
the purpose of, but just appending libfreebl3 to shlib-versions seems to
have worked.

I'm concerned about the fact test-installation.pl is only run at the end
of `make install'; if it fails at that point, you're screwed anyway, so
it seems to be of limited utility.  I wonder if it's possible to adapt
it to run after a `make install' with prefix!=/usr or DESTDIR set.  That
can be a separate patch, though, and I haven't played with it.

I can resubmit this to the list as an RFC if you think that would help
visibility.  Testing requires a special setup, and I'm not sure how much
of the automated testing infrastructure out there actually exercises
this script, or would be affected by the attached patch's changes.  The
length of the path from shlib-versions to test-installation.pl suggests
to me there are other things that depend on the framework.

Rical
diff --git a/Makeconfig b/Makeconfig
index 80c498e33b..d9e9003c4b 100644
--- a/Makeconfig
+++ b/Makeconfig
@@ -1110,6 +1110,8 @@ $(common-objpfx)soversions.mk: $(common-objpfx)soversions.i $(..)Makeconfig
 	   case $$number in \
 	     [0-9]*) echo "$$lib.so-version=.$$number"; \
 		     echo "all-sonames+=$$lib=$$lib.so\$$($$lib.so-version)";;\
+	     undef)  echo "$$lib.so-version="; \
+		     echo "all-sonames+=$$lib=$$lib.so";;\
 	     *)	     echo "$$lib.so-version=$$number"; \
 		     echo "all-sonames+=$$lib=\$$($$lib.so-version)";;\
 	   esac; \
diff --git a/scripts/soversions.awk b/scripts/soversions.awk
index 247f061bc3..676c8692f6 100644
--- a/scripts/soversions.awk
+++ b/scripts/soversions.awk
@@ -14,6 +14,8 @@ $1 == "DEFAULT" {
   sub(/=.*$/, "", lib);
   sub(/^.*=/, "", number);
   if (lib in numbers) next;
+  if (number == "")
+    number = "undef";
   numbers[lib] = number;
   order[lib] = ++order_n;
   if (NF > 1) {
diff --git a/scripts/test-installation.pl b/scripts/test-installation.pl
index 45c666b0a2..b77d7c4020 100755
--- a/scripts/test-installation.pl
+++ b/scripts/test-installation.pl
@@ -112,7 +112,8 @@ while (<SOVERSIONS>) {
   next if (/^all-sonames/);
   chop;
   if (/^lib/) {
-    ($name, $version)= /^lib(.*)\.so-version=\.(.*)$/;
+    ($name, $version)= /^lib(.*)\.so-version=\.?(.*)$/;
+    $version = "" if ! defined $version;
     # Filter out some libraries we don't want to link:
     # - nss_ldap since it's not yet available
     # - libdb1 since it conflicts with libdb
@@ -123,7 +124,8 @@ while (<SOVERSIONS>) {
     next if ($build_mathvec == 0 && $name eq "mvec");
     if ($name ne "nss_ldap" && $name ne "db1"
 	&& $name ne "thread_db"
-	&& $name ne "nss_test1" && $name ne "libgcc_s") {
+	&& $name ne "nss_test1" && $name ne "nss_test2"
+	&& $name ne "libgcc_s") {
       $link_libs .= " -l$name";
       $versions{$name} = $version;
     }
@@ -171,14 +173,20 @@ if ($?) {
 
 $ok = 1;
 %found = ();
+$librgx = qr/lib(\w+)\.so(?:\.([0-9\.]+))?/;
 
 open LDD, "ldd /tmp/test-prg$$ |"
   or die ("Couldn't execute ldd");
 while (<LDD>) {
-  if (/^\s*lib/) {
+  if (m,^\s*${librgx}\s*=>\s*(?:/[^/]+)*/${librgx},) {
     ($name, $version1, $version2) =
-      /^\s*lib(\w*)\.so\.([0-9\.]*)\s*=>.*\.so\.([0-9\.]*)/;
+      ($1, defined($2) ? $2 : '', defined($4) ? $4 : '');
     $found{$name} = 1;
+    if (!exists $versions{$name}) {
+      print "Unexpected dependency: lib$name\n";
+      $ok = 0;
+      next;
+    }
     if ($versions{$name} ne $version1 || $version1 ne $version2) {
       print "Library lib$name is not correctly installed.\n";
       print "Please check your installation!\n";
diff --git a/shlib-versions b/shlib-versions
index b9cb99d2fb..e73d4270ac 100644
--- a/shlib-versions
+++ b/shlib-versions
@@ -75,3 +75,7 @@ libgcc_s=1
 
 # The vector math library
 libmvec=1
+
+# libcrypt is linked against this when built with --enable-nss-crypt.
+# It does not use a versioned soname.
+libfreebl3=
Rical Jasan Feb. 7, 2018, 6:02 a.m. | #7
Ping.

On 12/19/2017 01:16 AM, Rical Jasan wrote:
> On 12/14/2017 07:10 AM, Florian Weimer wrote:
>> On 10/18/2017 07:06 AM, Rical Jasan wrote:
>>> diff --git a/scripts/test-installation.pl b/scripts/test-installation.pl
>>> index 45c666b0a2..fcb7707889 100755
>>> --- a/scripts/test-installation.pl
>>> +++ b/scripts/test-installation.pl
>>> @@ -171,14 +171,20 @@ if ($?) {
>>>     $ok = 1;
>>>   %found = ();
>>> +$librgx = qr/lib(\w+)\.so(?:\.([0-9\.]+))?/;
>>>     open LDD, "ldd /tmp/test-prg$$ |"
>>>     or die ("Couldn't execute ldd");
>>>   while (<LDD>) {
>>> -  if (/^\s*lib/) {
>>> +  if (m,^\s*${librgx}\s*=>\s*(?:/[^/]+)*/${librgx},) {
>>>       ($name, $version1, $version2) =
>>> -      /^\s*lib(\w*)\.so\.([0-9\.]*)\s*=>.*\.so\.([0-9\.]*)/;
>>> +      ($1, defined($2) ? $2 : '', defined($4) ? $4 : '');
>>>       $found{$name} = 1;
>>> +    if (!exists $versions{$name}) {
>>> +      print "Unexpected dependency: lib$name\n";
>>> +      $ok = 0;
>>> +      next;
>>> +    }
>>
>> This looks fine to me.  Could you commit it with a ChangeLog entry?
> 
> I have the resources available to set up burner systems now, so after
> reacquainting myself with this patch, I set about testing it.  Not only
> is test-installation.pl the very last thing run during `make install',
> but it _only_ happens when prefix=/usr and DESTDIR is not set, so even
> testing the patch requires bombing the system libc.
> 
> The above tidbit actually makes the situation worse because instead of
> having some "uninitialized value" warnings printed to stderr by Perl
> with an otherwise successful installation, that "Unexpected dependency"
> check will cause `make install' to fail, at the very end, after
> everything has already been installed.
> 
> What needs to happen is to make libfreebl3 an expected dependency, as
> well as allowing for unversioned sonames.  I've attached a patch that
> works for me, but it definitely stems from hacking just enough to make
> it work.  There are some design choices to be made, so others need to
> weigh in on whether they think what I did is appropriate.
> 
> The process starts with shlib-versions, which defines valid versions of
> shared libraries, both installed by glibc as well as external
> dependencies.  Note that in this file, versions are not prefixed by a
> dot (important later).  The choice I made here was to simply not define
> a version.
> 
> scripts/soversions.awk uses shlib-versions.v to create soversions.i in
> the build root.  I'm not sure where shlib-versions.v comes from, but it
> is a generated file as well.  I chose to tag the special case of no
> version with an arbitrary version of "undef" because if it is left blank
> at this point, the whitespace-delimited fields in soversions.i will be
> parsed incorrectly when generating soversions.mk in Makeconfig.
> 
> Correspondingly, Makeconfig is given a special case for the "undef"
> version, which generates soversions.mk lines absent of any versioning.
> There is some interesting voodoo here with what appears to be a
> superfluous dot preceding the version, but I tried stripping it to
> condense things and wound up with a broken installation with programs
> that couldn't find "libc.so6" at runtime, so I was convinced I should
> keep my unversioned soname propagation as an independent, exceptional case.
> 
> Lastly, scripts/test-installation.pl populates its %versions hash by
> matching lines in soversions.mk with versions prefixed with a dot, so I
> made the dot optional.  This results in an empty string for the version
> of unversioned libraries, but that is specific to the library name, and
> should be OK.  This is where we meet the above diff, which will match an
> unversioned library, so those empty strings will result in equality for
> a given library, and since an entry was defined for the library in the
> %versions hash, it isn't an unexpected dependency either.
> 
> There is some ordering in soversions.awk that I don't quite understand
> the purpose of, but just appending libfreebl3 to shlib-versions seems to
> have worked.
> 
> I'm concerned about the fact test-installation.pl is only run at the end
> of `make install'; if it fails at that point, you're screwed anyway, so
> it seems to be of limited utility.  I wonder if it's possible to adapt
> it to run after a `make install' with prefix!=/usr or DESTDIR set.  That
> can be a separate patch, though, and I haven't played with it.
> 
> I can resubmit this to the list as an RFC if you think that would help
> visibility.  Testing requires a special setup, and I'm not sure how much
> of the automated testing infrastructure out there actually exercises
> this script, or would be affected by the attached patch's changes.  The
> length of the path from shlib-versions to test-installation.pl suggests
> to me there are other things that depend on the framework.
> 
> Rical
Florian Weimer May 16, 2018, 11:08 a.m. | #8
On 12/19/2017 10:16 AM, Rical Jasan wrote:
> diff --git a/Makeconfig b/Makeconfig
> index 80c498e33b..d9e9003c4b 100644
> --- a/Makeconfig
> +++ b/Makeconfig
> @@ -1110,6 +1110,8 @@ $(common-objpfx)soversions.mk: $(common-objpfx)soversions.i $(..)Makeconfig
>   	   case $$number in \
>   	     [0-9]*) echo "$$lib.so-version=.$$number"; \
>   		     echo "all-sonames+=$$lib=$$lib.so\$$($$lib.so-version)";;\
> +	     undef)  echo "$$lib.so-version="; \
> +		     echo "all-sonames+=$$lib=$$lib.so";;\
>   	     *)	     echo "$$lib.so-version=$$number"; \
>   		     echo "all-sonames+=$$lib=\$$($$lib.so-version)";;\
>   	   esac; \
> diff --git a/scripts/soversions.awk b/scripts/soversions.awk
> index 247f061bc3..676c8692f6 100644
> --- a/scripts/soversions.awk
> +++ b/scripts/soversions.awk
> @@ -14,6 +14,8 @@ $1 == "DEFAULT" {
>     sub(/=.*$/, "", lib);
>     sub(/^.*=/, "", number);
>     if (lib in numbers) next;
> +  if (number == "")
> +    number = "undef";
>     numbers[lib] = number;
>     order[lib] = ++order_n;
>     if (NF > 1) {

I'm not sure if want to put libfrebl3.so into soversions.mk anyway.  It 
results in

gnu/lib-names-64.h:#define LIBFREEBL3_SO          "libfreebl3.so"

which is unexpected.

So perhaps it is better to stick libfreebl3.so explicitly into 
test-installation.pl.

> diff --git a/scripts/test-installation.pl b/scripts/test-installation.pl
> index 45c666b0a2..b77d7c4020 100755
> --- a/scripts/test-installation.pl
> +++ b/scripts/test-installation.pl
> @@ -112,7 +112,8 @@ while (<SOVERSIONS>) {
>     next if (/^all-sonames/);
>     chop;
>     if (/^lib/) {
> -    ($name, $version)= /^lib(.*)\.so-version=\.(.*)$/;
> +    ($name, $version)= /^lib(.*)\.so-version=\.?(.*)$/;
> +    $version = "" if ! defined $version;

I believe $version will always be defined at this point, although it can 
be the empty string.

>       # Filter out some libraries we don't want to link:
>       # - nss_ldap since it's not yet available
>       # - libdb1 since it conflicts with libdb
> @@ -123,7 +124,8 @@ while (<SOVERSIONS>) {
>       next if ($build_mathvec == 0 && $name eq "mvec");
>       if ($name ne "nss_ldap" && $name ne "db1"
>   	&& $name ne "thread_db"
> -	&& $name ne "nss_test1" && $name ne "libgcc_s") {
> +	&& $name ne "nss_test1" && $name ne "nss_test2"
> +	&& $name ne "libgcc_s") {

I think you need to filter out libfreebl3 at this point as well because 
it is not necessarily available.  It should not be subject to link 
tests, just like libgcc_s.

>         $link_libs .= " -l$name";
>         $versions{$name} = $version;
>       }
> @@ -171,14 +173,20 @@ if ($?) {
>   
>   $ok = 1;
>   %found = ();
> +$librgx = qr/lib(\w+)\.so(?:\.([0-9\.]+))?/;
>   
>   open LDD, "ldd /tmp/test-prg$$ |"
>     or die ("Couldn't execute ldd");
>   while (<LDD>) {
> -  if (/^\s*lib/) {
> +  if (m,^\s*${librgx}\s*=>\s*(?:/[^/]+)*/${librgx},) {
>       ($name, $version1, $version2) =
> -      /^\s*lib(\w*)\.so\.([0-9\.]*)\s*=>.*\.so\.([0-9\.]*)/;
> +      ($1, defined($2) ? $2 : '', defined($4) ? $4 : '');
>       $found{$name} = 1;
> +    if (!exists $versions{$name}) {
> +      print "Unexpected dependency: lib$name\n";
> +      $ok = 0;
> +      next;
> +    }
>       if ($versions{$name} ne $version1 || $version1 ne $version2) {
>         print "Library lib$name is not correctly installed.\n";
>         print "Please check your installation!\n";
> diff --git a/shlib-versions b/shlib-versions
> index b9cb99d2fb..e73d4270ac 100644
> --- a/shlib-versions
> +++ b/shlib-versions
> @@ -75,3 +75,7 @@ libgcc_s=1
>   
>   # The vector math library
>   libmvec=1
> +
> +# libcrypt is linked against this when built with --enable-nss-crypt.
> +# It does not use a versioned soname.
> +libfreebl3=

See above, this won't be necessary if libfreebl3.so is included in 
test-installation.pl.

Thanks,
Florian

Patch

diff --git a/scripts/test-installation.pl b/scripts/test-installation.pl
index 4b0e9f3c4a..466c526cc9 100755
--- a/scripts/test-installation.pl
+++ b/scripts/test-installation.pl
@@ -177,10 +177,15 @@  open LDD, "ldd /tmp/test-prg$$ |"
   or die ("Couldn't execute ldd");
 while (<LDD>) {
   if (/^\s*lib/) {
+    # When libcrypt is linked against NSS, some of the referenced
+    # libraries do not have a trailing version in their soname.
     ($name, $version1, $version2) =
-      /^\s*lib(\w*)\.so\.([0-9\.]*)\s*=>.*\.so\.([0-9\.]*)/;
+      /^\s*lib(\w*)\.so(?:\.([0-9\.]*))?\s*=>.*\.so(?:\.([0-9\.]*))?/;
     $found{$name} = 1;
-    if ($versions{$name} ne $version1 || $version1 ne $version2) {
+    if (defined($version1) != defined($version2)
+	|| defined($version1) != defined($versions{$name})
+	|| (defined($versions{$name})
+	    && ($versions{$name} ne $version1 || $version1 ne $version2))) {
       print "Library lib$name is not correctly installed.\n";
       print "Please check your installation!\n";
       print "Offending line of ldd output: $_\n";