shellcheck icon indicating copy to clipboard operation
shellcheck copied to clipboard

Writeable directory test passes with undefined variable

Open abitrolly opened this issue 1 year ago • 6 comments

For bugs

  • Rule Id (if any, e.g. SC1000): SC2086
  • My shellcheck version (shellcheck --version or 'online'): online
  • [x] I tried on shellcheck.net and verified that this is still a problem on the latest commit
  • [ ] It's not reproducible on shellcheck.net, but I think that's because it's an OS, configuration or encoding issue

For new checks and feature suggestions

  • [x] shellcheck.net (i.e. the latest commit) currently gives no useful warnings about this
  • [ ] I searched through https://github.com/koalaman/shellcheck/issues and didn't find anything related

Here's a snippet or screenshot that shows the problem:

#!/bin/sh
  
if ! [ -w $DEPLOYDIR ]; then
  echo "DEPLOYDIR needs to be set and writable"
  exit 1
fi

Here's what shellcheck currently says:

Line 3	SC2086: Double quote to prevent globbing and word splitting.

Here's what I wanted or expected to see:

The problem with the code that if the $DEPLOYDIR is undefined, or empty, the -w $DEPLOYDIR test passes. While shellcheck recommendation to add quotes help to fix the issue, the wiki page for SC2086 does not explain what is going on.

abitrolly avatar Jun 22 '24 05:06 abitrolly

It is normal, by default it doesn't alert about undefined uppercase variables (that are usually global), unless you enable the check. You can also choose to enable all checks by putting this at the start of the script: # shellcheck enable=all

ale5000-git avatar Jun 22 '24 22:06 ale5000-git

@ale5000-git if the variable is empty or undefined, what should be the result of the script? It tests if directory referenced by variable is writable, and if it is not writable, the script should show the error message. When variable is undefined it doesn't show the error, while the directory clearly doesn't exist. And this is the behavior what don't have any explanation for.

abitrolly avatar Jun 23 '24 07:06 abitrolly

@abitrolly

It is simply how the shell is, by using unquoted vars you will always get problems. The shell doesn't behave like a programming language where parameters are strictly defined.

So if you quote the vars:

if ! [ -w "$DEPLOYDIR" ]; then
  echo "DEPLOYDIR needs to be set and writable"
  exit 1
fi

when it is empty it will become like this:

if ! [ -w "" ]; then
  echo "DEPLOYDIR needs to be set and writable"
  exit 1
fi

and behave correctly.

Instead if it isn't quoted it became like this:

if ! [ -w  ]; then
  echo "DEPLOYDIR needs to be set and writable"
  exit 1
fi

so you no longer check anything and this is why the error isn't showed. To be more clear -w without parameters return true (0).

ale5000-git avatar Jun 24 '24 06:06 ale5000-git

-w without parameters return true.

Well, not really. It returns 0.

[ ] is an alias to call test command. From man test, the -w should fail (return 1) if dir does not exist.

       -w FILE
              FILE exists and the user has write access

But the man doesn't specify why it returns success (0) when parameter is missing and fail (1) when it is empty.

# without parameter
test -w   ; echo $? 
0

# quotes
test -w ""; echo $?
1

# current directory
test -w .; echo $?
0

# existing writable directory
test -w /tmp; echo $?
0

# existing read only directory
test -w /etc; echo $?
1

# invalid directory
test -w blablabla; echo $?
1

Missing parameter succeeds, but empty one (quotes) doesn't. Normally I would treat empty parameter "" as a current directory, but sh doesn't work this way.

This behavior needs to be explained. Applying generic quote check is good for a fix, but not sufficient for understanding. SC2086 with Double quote to prevent globbing and word splitting. is not quite right here.

abitrolly avatar Jun 24 '24 07:06 abitrolly

This is a "static analysis tool", it isn't a shell nor a shell manual; it just give general hints to help do better programming.

How already said you can use this to display all possible issues:

#!/bin/sh
# shellcheck enable=all

if ! [ -w $DEPLOYDIR ]; then
  echo "DEPLOYDIR needs to be set and writable"
  exit 1
fi

ale5000-git avatar Jun 24 '24 17:06 ale5000-git

@ale5000-git understood. So the hints should not be misleading.

abitrolly avatar Jun 24 '24 20:06 abitrolly