From 093df8cb2448a3b2f352b4e3105d2af45ccf3206 Mon Sep 17 00:00:00 2001 From: Christian Nassif-Haynes Date: Mon, 6 Sep 2021 05:52:34 +1000 Subject: [PATCH] Add extra checks for masked return codes --- src/ShellCheck/Analytics.hs | 109 ++++++++++++++++++++++++++++++ src/ShellCheck/AnalyzerLib.hs | 18 +++++ src/ShellCheck/Checks/Commands.hs | 2 - src/ShellCheck/Data.hs | 2 + 4 files changed, 129 insertions(+), 2 deletions(-) diff --git a/src/ShellCheck/Analytics.hs b/src/ShellCheck/Analytics.hs index de59be1..a09e804 100644 --- a/src/ShellCheck/Analytics.hs +++ b/src/ShellCheck/Analytics.hs @@ -260,6 +260,13 @@ optionalTreeChecks = [ cdPositive = "set -e; func() { cp *.txt ~/backup; rm *.txt; }; func && echo ok", cdNegative = "set -e; func() { cp *.txt ~/backup; rm *.txt; }; func; echo ok" }, checkSetESuppressed) + + ,(newCheckDescription { + cdName = "check-extra-masked-returns", + cdDescription = "Check for additional cases where exit codes are masked", + cdPositive = "rm -r \"$(get_chroot_dir)/home\"", + cdNegative = "set -e; dir=\"$(get_chroot_dir)\"; rm -r \"$dir/home\"" + }, checkExtraMaskedReturns) ] optionalCheckMap :: Map.Map String (Parameters -> Token -> [TokenComment]) @@ -4734,5 +4741,107 @@ checkSetESuppressed params t = isIn t cmds = getId t `elem` map getId cmds +prop_checkExtraMaskedReturns1 = verifyTree checkExtraMaskedReturns "cat < <(ls)" +prop_checkExtraMaskedReturns2 = verifyTree checkExtraMaskedReturns "read -ra arr <(ls)" +prop_checkExtraMaskedReturns3 = verifyTree checkExtraMaskedReturns "ls >(cat)" +prop_checkExtraMaskedReturns4 = verifyTree checkExtraMaskedReturns "false | true" +prop_checkExtraMaskedReturns5 = verifyNotTree checkExtraMaskedReturns "set -o pipefail; false | true" +prop_checkExtraMaskedReturns6 = verifyNotTree checkExtraMaskedReturns "false | true || true" +prop_checkExtraMaskedReturns7 = verifyTree checkExtraMaskedReturns "true $(false)" +prop_checkExtraMaskedReturns8 = verifyTree checkExtraMaskedReturns "x=$(false)$(true)" +prop_checkExtraMaskedReturns9 = verifyNotTree checkExtraMaskedReturns "x=$(false)true" +prop_checkExtraMaskedReturns10 = verifyTree checkExtraMaskedReturns "x=`false``false`" +prop_checkExtraMaskedReturns11 = verifyTree checkExtraMaskedReturns "x=\"$(false)$(true)\"" +prop_checkExtraMaskedReturns12 = verifyTree checkExtraMaskedReturns "x=\"$(false)\"\"$(true)\"" +prop_checkExtraMaskedReturns13 = verifyTree checkExtraMaskedReturns "true <<<$(false)" +prop_checkExtraMaskedReturns14 = verifyNotTree checkExtraMaskedReturns "echo asdf | false" +prop_checkExtraMaskedReturns15 = verifyNotTree checkExtraMaskedReturns "readonly x=$(false)" +prop_checkExtraMaskedReturns16 = verifyTree checkExtraMaskedReturns "readarray -t files < <(ls)" +prop_checkExtraMaskedReturns17 = verifyNotTree checkExtraMaskedReturns "x=( $(false) false )" +prop_checkExtraMaskedReturns18 = verifyTree checkExtraMaskedReturns "x=( $(false) $(false) )" +prop_checkExtraMaskedReturns19 = verifyNotTree checkExtraMaskedReturns "x=( $(false) [4]=false )" +prop_checkExtraMaskedReturns20 = verifyTree checkExtraMaskedReturns "x=( $(false) [4]=$(false) )" +prop_checkExtraMaskedReturns21 = verifyTree checkExtraMaskedReturns "cat << foo\n $(false)\nfoo" +prop_checkExtraMaskedReturns22 = verifyTree checkExtraMaskedReturns "[[ $(false) ]]" +prop_checkExtraMaskedReturns23 = verifyNotTree checkExtraMaskedReturns "x=$(false) y=z" +prop_checkExtraMaskedReturns24 = verifyNotTree checkExtraMaskedReturns "x=$(( $(date +%s) ))" +prop_checkExtraMaskedReturns25 = verifyTree checkExtraMaskedReturns "echo $(( $(date +%s) ))" +prop_checkExtraMaskedReturns26 = verifyNotTree checkExtraMaskedReturns "x=( $(false) )" +prop_checkExtraMaskedReturns27 = verifyTree checkExtraMaskedReturns "x=$(false) false" +prop_checkExtraMaskedReturns28 = verifyTree checkExtraMaskedReturns "x=$(false) y=$(false)" +prop_checkExtraMaskedReturns29 = verifyNotTree checkExtraMaskedReturns "false < <(set -e)" +prop_checkExtraMaskedReturns30 = verifyNotTree checkExtraMaskedReturns "false < <(shopt -s cdspell)" +prop_checkExtraMaskedReturns31 = verifyNotTree checkExtraMaskedReturns "false < <(dirname \"${BASH_SOURCE[0]}\")" +prop_checkExtraMaskedReturns32 = verifyNotTree checkExtraMaskedReturns "false < <(basename \"${BASH_SOURCE[0]}\")" +prop_checkExtraMaskedReturns33 = verifyNotTree checkExtraMaskedReturns "{ false || true; } | true" +prop_checkExtraMaskedReturns34 = verifyNotTree checkExtraMaskedReturns "{ false || :; } | true" +checkExtraMaskedReturns params t = runNodeAnalysis findMaskingNodes params t + where + findMaskingNodes _ (T_Arithmetic _ list) = findMaskedNodesInList [list] + findMaskingNodes _ (T_Array _ list) = findMaskedNodesInList $ allButLastSimpleCommands list + findMaskingNodes _ (T_Condition _ _ condition) = findMaskedNodesInList [condition] + findMaskingNodes _ (T_DoubleQuoted _ list) = findMaskedNodesInList $ allButLastSimpleCommands list + findMaskingNodes _ (T_HereDoc _ _ _ _ list) = findMaskedNodesInList list + findMaskingNodes _ (T_HereString _ word) = findMaskedNodesInList [word] + findMaskingNodes _ (T_NormalWord _ parts) = findMaskedNodesInList $ allButLastSimpleCommands parts + findMaskingNodes _ (T_Pipeline _ _ cmds) | not (hasPipefail params) = findMaskedNodesInList $ allButLastSimpleCommands cmds + findMaskingNodes _ (T_ProcSub _ _ list) = findMaskedNodesInList list + findMaskingNodes _ (T_SimpleCommand _ assigns (_:args)) = findMaskedNodesInList $ assigns ++ args + findMaskingNodes _ (T_SimpleCommand _ assigns []) = findMaskedNodesInList $ allButLastSimpleCommands assigns + findMaskingNodes _ _ = return () + + findMaskedNodesInList = mapM_ (doAnalysis findMaskedNodes) + + isMaskedNode t = not (isHarmlessCommand t || isCheckedElsewhere t || isMaskDeliberate t) + findMaskedNodes t@(T_SimpleCommand _ _ (_:_)) = when (isMaskedNode t) $ inform t + findMaskedNodes t@T_Condition {} = when (isMaskedNode t) $ inform t + findMaskedNodes _ = return () + + containsSimpleCommand t = isNothing $ doAnalysis go t + where + go t = case t of + T_SimpleCommand {} -> fail "" + _ -> return () + + allButLastSimpleCommands cmds = + if null simpleCommands then [] else init simpleCommands + where + simpleCommands = filter containsSimpleCommand cmds + + inform t = info (getId t) 2312 ("Consider invoking this command " + ++ "separately to avoid masking its return value (or use '|| true' " + ++ "to ignore).") + + isMaskDeliberate t = hasParent isOrIf t + where + isOrIf _ (T_OrIf _ _ (T_Pipeline _ _ [T_Redirecting _ _ cmd])) + = getCommandBasename cmd `elem` [Just "true", Just ":"] + isOrIf _ _ = False + + isCheckedElsewhere t = hasParent isDeclaringCommand t + where + isDeclaringCommand t _ = fromMaybe False $ do + basename <- getCommandBasename t + return $ basename `elem` declaringCommands + + isHarmlessCommand t = fromMaybe False $ do + basename <- getCommandBasename t + return $ basename `elem` [ + "echo" + ,"basename" + ,"dirname" + ,"printf" + ,"set" + ,"shopt" + ] + + parentChildPairs t = go $ parents params t + where + go (child:parent:rest) = (parent, child):go (parent:rest) + go _ = [] + + hasParent pred t = any (uncurry pred) (parentChildPairs t) + + return [] runTests = $( [| $(forAllProperties) (quickCheckWithResult (stdArgs { maxSuccess = 1 }) ) |]) diff --git a/src/ShellCheck/AnalyzerLib.hs b/src/ShellCheck/AnalyzerLib.hs index 9cb6cd2..be54fbb 100644 --- a/src/ShellCheck/AnalyzerLib.hs +++ b/src/ShellCheck/AnalyzerLib.hs @@ -83,6 +83,8 @@ data Parameters = Parameters { hasInheritErrexit :: Bool, -- Whether this script has 'set -e' anywhere. hasSetE :: Bool, + -- Whether this script has 'set -o pipefail' anywhere. + hasPipefail :: Bool, -- A linear (bad) analysis of data flow variableFlow :: [StackData], -- A map from Id to parent Token @@ -204,6 +206,12 @@ makeParameters spec = Dash -> True Sh -> True Ksh -> False, + hasPipefail = + case shellType params of + Bash -> containsPipefail root + Dash -> True + Sh -> True + Ksh -> containsPipefail root, shellTypeSpecified = isJust (asShellType spec) || isJust (asFallbackShell spec), parentMap = getParentTree root, variableFlow = getVariableFlow params root, @@ -226,6 +234,16 @@ containsSetE root = isNothing $ doAnalysis (guard . not . isSetE) root _ -> False re = mkRegex "[[:space:]]-[^-]*e" +containsPipefail root = isNothing $ doAnalysis (guard . not . isPipefail) root + where + isPipefail t = + case t of + T_SimpleCommand {} -> + t `isUnqualifiedCommand` "set" && + ("pipefail" `elem` oversimplify t || + "o" `elem` map snd (getAllFlags t)) + _ -> False + containsShopt shopt root = isNothing $ doAnalysis (guard . not . isShoptLastPipe) root where diff --git a/src/ShellCheck/Checks/Commands.hs b/src/ShellCheck/Checks/Commands.hs index d0ada73..77bdbf6 100644 --- a/src/ShellCheck/Checks/Commands.hs +++ b/src/ShellCheck/Checks/Commands.hs @@ -101,8 +101,6 @@ commandChecks = [ ++ map checkArgComparison declaringCommands ++ map checkMaskedReturns declaringCommands -declaringCommands = ["local", "declare", "export", "readonly", "typeset", "let"] - optionalChecks = map fst optionalCommandChecks optionalCommandChecks :: [(CheckDescription, CommandCheck)] diff --git a/src/ShellCheck/Data.hs b/src/ShellCheck/Data.hs index 793a4de..e22b424 100644 --- a/src/ShellCheck/Data.hs +++ b/src/ShellCheck/Data.hs @@ -138,3 +138,5 @@ shellForExecutable name = _ -> Nothing flagsForRead = "sreu:n:N:i:p:a:t:" + +declaringCommands = ["local", "declare", "export", "readonly", "typeset", "let"]