415767 - pkits.sh compares optional variables with numeric values.

Status: RESOLVED FIXED

(NSS :: Test, defect)

Description

[Slavomir Katuscak]

In pkits.sh there are more constructions like:
if [ "$NSS_NO_PKITS_CRLS" -ne 1 ]; then

When compared variable doesn't exists (it's optional) then this comparison fails with error message.

I suggest to use rather content check:
if [ -z "$NSS_NO_PKITS_CRLS" ]; then

Comment 1

[Slavomir Katuscak]

Attachment: Patch.

This patch fixes described problem + replaces syntax $( ) with ` ` (caused some problems on default shell on Solaris 11). I use this version already on my test machine.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

I suggest another possible solution.  I suggest we stop using Solaris's 
default shell, and start using BASH or some other truly POSIX shell.  
The fact that Solaris's shell doesn't support POSIX's superior $( ) syntax
is, IMO, a deficiency (a bug) in it.  Rather than dumbing down our scripts
on all unix platforms to accomodate it, I suggest we look into using a real
POSIX shell on Solaris.

Comment 3

[Julien Pierre]

Slavo, is this problem specific to Solaris 11 ? If so, maybe that's where it needs to be fixed. Please report a bug against it rather than NSS.

Comment 4

[Slavomir Katuscak]

(In reply to comment #2)
> I suggest another possible solution.  I suggest we stop using Solaris's 
> default shell, and start using BASH or some other truly POSIX shell.  
> The fact that Solaris's shell doesn't support POSIX's superior $( ) syntax
> is, IMO, a deficiency (a bug) in it.  Rather than dumbing down our scripts
> on all unix platforms to accomodate it, I suggest we look into using a real
> POSIX shell on Solaris.
> 

Only pkits.sh uses $( ) syntax, all other scripts in tests directory uses ` `. After changing first line in all.sh to #!/bin/bash syntax $( ) passes. The question is shell change wouldn't cause problems in other scripts.

(In reply to comment #3)
> Slavo, is this problem specific to Solaris 11 ? If so, maybe that's where it
> needs to be fixed. Please report a bug against it rather than NSS.
> 

No, it's the same on Solaris 10 (checked now on machine simplify). It was not detected before, because we don't run this script in nightly tests or Tinderboxes.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

We've run into this issue with $( ) being unsupported in Solaris shell before.
We keep converting them to ` ` when we do.  I'd like us to stop doing that,
and just use $( ) and real POSIX shells.

Comment 6

[Julien Pierre]

I'm not sure what other changes to the script this will entail, but Solaris 10 ships with bash 3.00 in /usr/bin/bash , and the man page says it's POSIX compliant.

Comment 7

[Slavomir Katuscak]

We use many different platforms, and we don't have bash on all of them (we don't have it for example on Win64 Tinderbox, but I think there exists version for MSYS), I'm also not sure about AIX or HPUX. But also in case we have bash on all supported platforms, we can't rely that it is in the same directory. It can be /bin/bash, /usr/bin/bash, /usr/local/bin/bash,... This causes only complications, because we need to specify shell path on first line of scripts (or just remove this line). Change $( ) to ` ` is simple and it's also more clear to use one method in all scripts.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

We don't need bash on all platforms.  We need a POSIX shell on all platforms.
MKS is a POSIX shell on Windows.  Other platforms own /bin/sh shells may be
(and typically are, I think) POSIX shells.  AFAIK, Solaris is the exception.

I think this can be resolved simply by requiring that on each system that
runs all.sh, in the environment of the user who is running all.sh, the SHELL environment variable must be set to the path name of a POSIX shell for that platform.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 301492 [details] [diff] [review]
Patch.

Slavo,

This change, and the others like it, are all OK.

>-  if [ "$NSS_NO_PKITS_CRLS" -ne 1 ]; then
>+  if [ -z "$NSS_NO_PKITS_CRLS" ]; then

But this change, and the others like are IMO far less desirable
than using a POSIX shell (one that understands $( ) ).

>-  RET=$(expr $RET + $(grep -c ERROR ${PKITSDIR}/cmdout.txt))
>+  RET=`expr $RET + `grep -c ERROR ${PKITSDIR}/cmdout.txt``

So, please see if we can get our Solaris build systems to build 
using a POSIX shell, and if so, switch the builds to use that,
then submit another patch that doesn't change $( ) to ` `.
If it is not feasible to use a POSIX shell, let me know that, too.

Comment 10

[Slavomir Katuscak]

Nelson, I don't think that enforcing POSIX shell is the best solution. If there was one shell (like /bin/bash) on all platforms we can set it in first line of scripts, but it isn't, so we should explicitly set it when running all.sh (add there exception for Solaris to our local scripts to use /bin/bash). This will also cause complications in Tinderboxes and users used to run only ./all.sh would have to run /bin/bash all.sh. This is big complication, I think replacing syntax is easier, and code will stay POSIX compliant.

Do you have any better idea how to run scripts in POSIX shell without complications like this ?

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Slavo, some shell scripot invokes all.sh.  The script that does so must simply
invoke a POSIX shell first, and set the SHELL variable to point to that shell.
There is no need to make ANY change to all.sh for this, and I would not approve
a proposed change to all.sh for this purpose.

Comment 12

[Slavomir Katuscak]

If there is not unique shell for all architecture it's only complication for us to modify scripts to detect OS and select shell. Discussed with Christophe yesterday, we rather want to use ` `. It's very simple change and code stays POSIX compliant. 

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Use of backquotes cannot be nested.  Use of $( ) can be.  
The expression that you coded (and to which I objected) probably does 
not work as you expected it to.

The original command has two nested expressions.

>-  RET=$(expr $RET + $(grep -c ERROR ${PKITSDIR}/cmdout.txt) )
         ^             ^                                    ^ ^
         |             +---- inner expression --------------+ |
         +----------- outer expression -----------------------+

But your replacement command has two non-nested expressions. 

>+  RET=`expr $RET + `grep -c ERROR ${PKITSDIR}/cmdout.txt``
        ^            ^                                    ^^
        +-- first ---+                                  second

That line should cause an error when interpreted.

So, don't think of my insistence on using a shell that supports $( )
(which all POSIX shells should do) are a mere request.  Think of it as a
requirement for NSS shell scripts going forward.  

Again, the place to establish the shell being used for all.sh is NOT inside 
of all.sh or shells that it invokes.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Typo, I meant to write "... AS a mere request."

Of course, another way to fix the above is to change that line to two 
lines, e.g. 
    FOO=`grep -c ERROR ${PKITSDIR}/cmdout.txt`
    RET=`expr $RET + $FOO`

But clearly those backquotes are a potential trap, and your patch fell into it.
So, let's avoid any more use of backquotes (a.k.a grave's) and stick with sane
nestable expression delimiters.

Comment 15

[Slavomir Katuscak]

Nelson,

Thanks for clarification of nested backquotes, it's really a trap I didn't noticed. 

But in case that we want to avoid backquotes and use POSIX shell on all tested platforms we need also:

-modify test_nss script on mace (running nightly tests) and add there shell detection by OS
-modify tinderbox code
-if there are any other scripts running all.sh, we need to modify them to ensure that they use POSIX shell

If we add new script or platform in future, we will always need to synchronize scripts (do the same work on more places) and that's what I would like to avoid and rather stay with backquotes. 

Comment 16

[Julien Pierre]

Can we find a fix for this that doesn't involve changing shells ? I use Solaris for my primary development and I would like to be able to run pkits.sh . Right now, the only thing I have is attachment 301492 [details] [diff] [review] which is probably not working as expected.

Comment 17

[Slavomir Katuscak]

Attachment: Patch v2.

Implemented suggestion from comment 14. I would rather stay compatible also with default shell on Solaris (this code is still POSIX compliant).

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

Julien, there's no reason that you can't continue to use Solaris as your 
primary development and continue to you your favorite shell (regardless of 
what it is).  The only thing you would need to do differently, when running
all.sh or any NSS script, would be to specify the shell, e.g. rather than
merely typing the command
   all.sh
or (as I do)
   all.sh 2>&1 | tee /tmp/all.sh.out

is to prefix the all.sh command with the name of the posix compliant shell, 
e.g. (assuming that /bin.bash is on your machine)
   /bin/bash -c all.sh 
or
   /bin/bash -c all.sh 2>&1 | tee > /tmp/all.sh.out

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

well, there were a few typos in the above, but you get the idea.
I meant /bin/bash, not /bin.bash  and that last command line should be
   /bin/bash -c all.sh 2>&1 | tee /tmp/all.sh.out

Comment 20

[Slavomir Katuscak]

Checking in pkits.sh;
/cvsroot/mozilla/security/nss/tests/pkits/pkits.sh,v  <--  pkits.sh
new revision: 1.19; previous revision: 1.18
done