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

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";