diff --git a/doc/cimple.md b/doc/cimple.md index 46d541e..774fb84 100644 --- a/doc/cimple.md +++ b/doc/cimple.md @@ -1,6 +1,6 @@ # Cimple-based linters (`check-cimple`) -There are currently 35 linters implemented, out of which 8 perform global analyses. +There are currently 36 linters implemented, out of which 8 perform global analyses. In the list below, the global ones are marked specially. ## `-Wassert` @@ -293,6 +293,34 @@ Checks that no escape sequences are present in the logger format string. to ensure that each log output is a single line. It's particularly easy to accidentally add `\n` to the end of a log format. This avoids that problem. +## `-Wmalloc-call` + +Checks that allocation functions like `mem_balloc` are always first assigned to +a local variable. The exception is in a return statement, e.g. in simple typed +allocation functions like `logger_new()`. If the allocation is stored in a local +variable, that variable must immediately be checked against `nullptr` before +doing anything else. + +Invalid code: + +```c +ob->mem = (My_Struct *)mem_alloc(mem, sizeof(My_Struct)); +``` + +Valid code: + +```c +My_Struct *tmp = (My_Struct *)mem_alloc(mem, sizeof(My_Struct)) +if (tmp == nullptr) { + return false; +} +ob->mem = tmp; +``` + +**Reason:** This avoids accidentally putting `nullptr` into a location without +checking first. Putting `nullptr` somewhere may be ok, but we must do it +intentionally. + ## `-Wmalloc-type` Checks that `mem_balloc` is only used for built-in types. For struct allocations diff --git a/doc/generate b/doc/generate new file mode 100755 index 0000000..d1bf84d --- /dev/null +++ b/doc/generate @@ -0,0 +1,7 @@ +#!/usr/bin/env bash + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" &>/dev/null && pwd)" + +set -eux + +bazel run //hs-tokstyle/tools:check-cimple -- --help >"$SCRIPT_DIR/cimple.md" diff --git a/src/Tokstyle/Common.hs b/src/Tokstyle/Common.hs index ab58575..1a484ac 100644 --- a/src/Tokstyle/Common.hs +++ b/src/Tokstyle/Common.hs @@ -3,9 +3,12 @@ module Tokstyle.Common ( functionName , isPointer , semEq + , skip + , (>+>) ) where import Data.Fix (Fix (..)) +import qualified Data.List as List import Data.Text (Text) import Language.Cimple (Lexeme (..), LexemeClass (..), Node, NodeF (..), removeSloc) @@ -38,3 +41,12 @@ functionName _ = Nothing -- Semantic equality: nodes are the same, except for source locations. semEq :: Node (Lexeme Text) -> Node (Lexeme Text) -> Bool semEq a b = removeSloc a == removeSloc b + + +-- Don't apply the linter to certain files. +skip :: [FilePath] -> (FilePath, [Node (Lexeme Text)]) -> (FilePath, [Node (Lexeme Text)]) +skip fps (fp, _) | any (`List.isSuffixOf` fp) fps = (fp, []) +skip _ tu = tu + +(>+>) :: Monad m => (t -> m ()) -> (t -> m ()) -> t -> m () +(>+>) f g x = f x >> g x diff --git a/src/Tokstyle/Linter.hs b/src/Tokstyle/Linter.hs index c56a7c8..f5a9a88 100644 --- a/src/Tokstyle/Linter.hs +++ b/src/Tokstyle/Linter.hs @@ -33,6 +33,7 @@ import qualified Tokstyle.Linter.LargeStructParams as LargeStructParams import qualified Tokstyle.Linter.LoggerCalls as LoggerCalls import qualified Tokstyle.Linter.LoggerConst as LoggerConst import qualified Tokstyle.Linter.LoggerNoEscapes as LoggerNoEscapes +import qualified Tokstyle.Linter.MallocCall as MallocCall import qualified Tokstyle.Linter.MallocType as MallocType import qualified Tokstyle.Linter.MemcpyStructs as MemcpyStructs import qualified Tokstyle.Linter.MissingNonNull as MissingNonNull @@ -83,6 +84,7 @@ localLinters = , LoggerCalls.descr , LoggerConst.descr , LoggerNoEscapes.descr + , MallocCall.descr , MallocType.descr , MemcpyStructs.descr , MissingNonNull.descr diff --git a/src/Tokstyle/Linter/CallocArgs.hs b/src/Tokstyle/Linter/CallocArgs.hs index f9403f0..1edee9c 100644 --- a/src/Tokstyle/Linter/CallocArgs.hs +++ b/src/Tokstyle/Linter/CallocArgs.hs @@ -8,10 +8,12 @@ import qualified Control.Monad.State.Strict as State import Data.Fix (Fix (..)) import Data.Text (Text) import qualified Data.Text as Text -import Language.Cimple (Lexeme (..), Node, NodeF (..)) +import Language.Cimple (BinaryOp (BopMul), Lexeme (..), + Node, NodeF (..)) import Language.Cimple.Diagnostics (warn) import Language.Cimple.TraverseAst (AstActions, astActions, doNode, traverseAst) +import qualified Tokstyle.Common as Common checkSize, checkNmemb :: Text -> FilePath -> Node (Lexeme Text) -> State [Text] () @@ -20,8 +22,9 @@ checkSize funName file size = case unFix size of _ -> warn file size $ "`size` argument in call to `" <> funName <> "` must be a sizeof expression" checkNmemb funName file nmemb = case unFix nmemb of - LiteralExpr{} -> return () - VarExpr{} -> return () + LiteralExpr{} -> return () + VarExpr{} -> return () + ParenExpr e -> checkNmemb funName file e PointerAccess e _ -> checkNmemb funName file e BinaryExpr l _ r -> do checkNmemb funName file l @@ -40,6 +43,12 @@ pattern Calloc funName args <- Fix (FunctionCall (Fix (VarExpr (L _ _ funName))) linter :: AstActions (State [Text]) Text linter = astActions { doNode = \file node act -> case node of + Calloc funName@"calloc" [nmemb, size] -> do + checkNmemb funName file nmemb + checkSize funName file size + Calloc funName@"realloc" [_, Fix (BinaryExpr nmemb BopMul size)] -> do + checkNmemb funName file nmemb + checkSize funName file size Calloc funName@"mem_alloc" [_, size] -> do checkSize funName file size Calloc funName@"mem_valloc" [_, nmemb, size] -> do @@ -49,7 +58,9 @@ linter = astActions checkNmemb funName file nmemb checkSize funName file size - Calloc "mem_alloc" _ -> warn file node "invalid `mem_alloc` invocation: 1 arguments after `mem` expected" + Calloc "calloc" _ -> warn file node "invalid `calloc` invocation: 2 arguments expected" + Calloc "realloc" _ -> warn file node "invalid `realloc` invocation: 2 arguments expected" + Calloc "mem_alloc" _ -> warn file node "invalid `mem_alloc` invocation: 1 argument after `mem` expected" Calloc "mem_valloc" _ -> warn file node "invalid `mem_valloc` invocation: 2 arguments after `mem` expected" Calloc "mem_vrealloc" _ -> warn file node "invalid `mem_vrealloc` invocation: 3 argument after `mem` expected" @@ -57,7 +68,11 @@ linter = astActions } analyse :: (FilePath, [Node (Lexeme Text)]) -> [Text] -analyse = reverse . flip State.execState [] . traverseAst linter +analyse = reverse . flip State.execState [] . traverseAst linter . Common.skip + [ "toxav/rtp.c" + , "toxcore/list.c" + , "toxcore/mem.c" + ] descr :: ((FilePath, [Node (Lexeme Text)]) -> [Text], (Text, Text)) descr = (analyse, ("calloc-args", Text.unlines diff --git a/src/Tokstyle/Linter/CallocType.hs b/src/Tokstyle/Linter/CallocType.hs index 70fee3a..e67c153 100644 --- a/src/Tokstyle/Linter/CallocType.hs +++ b/src/Tokstyle/Linter/CallocType.hs @@ -13,6 +13,7 @@ import Language.Cimple.Diagnostics (warn) import Language.Cimple.Pretty (showNode) import Language.Cimple.TraverseAst (AstActions, astActions, doNode, traverseAst) +import qualified Tokstyle.Common as Common import Tokstyle.Common (semEq) @@ -32,6 +33,8 @@ pattern Calloc :: Text -> [Node (Lexeme Text)] -> Node (Lexeme Text) pattern Calloc funName args <- Fix (FunctionCall (Fix (VarExpr (L _ _ funName))) args) isCalloc :: Text -> Bool +isCalloc "calloc" = True +isCalloc "realloc" = True isCalloc "mem_alloc" = True isCalloc "mem_valloc" = True isCalloc "mem_vrealloc" = True @@ -40,6 +43,12 @@ isCalloc _ = False linter :: AstActions (State [Text]) Text linter = astActions { doNode = \file node act -> case node of + Fix (CastExpr castTy (Calloc funName@"calloc" [_, Fix (SizeofType sizeofTy)])) -> + checkTypes funName file castTy sizeofTy + Fix (CastExpr castTy (Calloc funName@"calloc" [_, Fix (BinaryExpr _ _ (Fix (SizeofType sizeofTy)))])) -> + checkTypes funName file castTy sizeofTy + Fix (CastExpr castTy (Calloc funName@"realloc" [_, Fix (BinaryExpr _ _ (Fix (SizeofType sizeofTy)))])) -> + checkTypes funName file castTy sizeofTy Fix (CastExpr castTy (Calloc funName@"mem_alloc" [_, Fix (SizeofType sizeofTy)])) -> checkTypes funName file castTy sizeofTy Fix (CastExpr castTy (Calloc funName@"mem_valloc" [_, _, Fix (SizeofType sizeofTy)])) -> @@ -54,7 +63,11 @@ linter = astActions } analyse :: (FilePath, [Node (Lexeme Text)]) -> [Text] -analyse = reverse . flip State.execState [] . traverseAst linter +analyse = reverse . flip State.execState [] . traverseAst linter . Common.skip + [ "toxav/rtp.c" + , "toxcore/list.c" + , "toxcore/mem.c" + ] descr :: ((FilePath, [Node (Lexeme Text)]) -> [Text], (Text, Text)) descr = (analyse, ("calloc-type", Text.unlines diff --git a/src/Tokstyle/Linter/MallocCall.hs b/src/Tokstyle/Linter/MallocCall.hs new file mode 100644 index 0000000..326d973 --- /dev/null +++ b/src/Tokstyle/Linter/MallocCall.hs @@ -0,0 +1,117 @@ +{-# OPTIONS_GHC -Wwarn #-} +{-# LANGUAGE OverloadedStrings #-} +{-# LANGUAGE PatternSynonyms #-} +{-# LANGUAGE Strict #-} +module Tokstyle.Linter.MallocCall (descr) where + +import Control.Monad.State.Strict (State) +import qualified Control.Monad.State.Strict as State +import Data.Fix (Fix (..)) +import Data.Text (Text) +import qualified Data.Text as Text +import Language.Cimple (Lexeme (..), Node, NodeF (..)) +import Language.Cimple.Diagnostics (warn) +import Language.Cimple.TraverseAst (AstActions, astActions, doNode, + doNodes, traverseAst) +import qualified Tokstyle.Common as Common +import Tokstyle.Common ((>+>)) + + +mallocFuncs :: [Text] +mallocFuncs = + [ "mem_balloc" + , "mem_alloc" + , "mem_valloc" + , "mem_vrealloc" + , "malloc" + , "calloc" + , "realloc" + ] + +pattern FunCall, FunctionCast :: text -> Node (Lexeme text) +pattern FunCall name <- Fix (FunctionCall (Fix (VarExpr (L _ _ name))) _) +pattern FunctionCast name <- Fix (CastExpr _ (FunCall name)) + +pattern MallocVarDecl :: text -> Node (Lexeme text) -> Node (Lexeme text) +pattern MallocVarDecl decl initialiser <- Fix (VarDeclStmt (Fix (VarDecl _ (L _ _ decl) _)) (Just initialiser)) + +pattern MallocReturn :: Node lexeme -> Node lexeme +pattern MallocReturn initialiser <- Fix (Return (Just initialiser)) + +lintAssign :: AstActions (State [Text]) Text +lintAssign = astActions + { doNode = \file node act -> case node of + MallocVarDecl _ (FunctionCast name) | name `elem` mallocFuncs -> return () + MallocReturn (FunctionCast name) | name `elem` mallocFuncs -> return () + -- We check in -Wcalloc-type that casts are done correctly. This avoids + -- double-warning on non-cast malloc calls. + MallocVarDecl _ (FunCall name) | name `elem` mallocFuncs -> return () + MallocReturn (FunCall name) | name `elem` mallocFuncs -> return () + + FunCall name | name `elem` mallocFuncs -> + warn file node $ "allocations using `" <> name + <> "` must first be assigned to a local variable or " + <> "returned directly" + + _ -> act + } + +pattern NullCheck :: Text -> Node (Lexeme Text) +pattern NullCheck ref <- + Fix (IfStmt + (Fix (BinaryExpr (Fix (VarExpr (L _ _ ref))) _ (Fix (VarExpr (L _ _ "nullptr"))))) + (Fix (CompoundStmt _)) _) + +lintCheck :: AstActions (State [Text]) Text +lintCheck = astActions + { doNodes = \file nodes act -> case nodes of + (MallocVarDecl decl FunctionCast{}:ss@(NullCheck ref:_)) | decl == ref -> + traverseAst lintCheck (file, ss) + (MallocVarDecl decl (FunctionCast name):s:_) -> + warn file s $ "`" <> decl <> "`, assigned from `" <> name + <> "` must immediately be checked against `nullptr`" + + _ -> act + } + +analyse :: (FilePath, [Node (Lexeme Text)]) -> [Text] +analyse = reverse . flip State.execState [] . (traverseAst lintCheck >+> traverseAst lintAssign) + -- TODO(iphydf): Refactor after the toxav PR. + . Common.skip + [ "toxav/audio.c" + , "toxav/groupav.c" + , "toxav/msi.c" + , "toxav/ring_buffer.c" + , "toxav/rtp.c" + , "toxav/toxav.c" + , "toxav/video.c" + ] + +descr :: ((FilePath, [Node (Lexeme Text)]) -> [Text], (Text, Text)) +descr = (analyse, ("malloc-call", Text.unlines + [ "Checks that allocation functions like `mem_balloc` are always first assigned to" + , "a local variable. The exception is in a return statement, e.g. in simple typed" + , "allocation functions like `logger_new()`. If the allocation is stored in a local" + , "variable, that variable must immediately be checked against `nullptr` before" + , "doing anything else." + , "" + , "Invalid code:" + , "" + , "```c" + , "ob->mem = (My_Struct *)mem_alloc(mem, sizeof(My_Struct));" + , "```" + , "" + , "Valid code:" + , "" + , "```c" + , "My_Struct *tmp = (My_Struct *)mem_alloc(mem, sizeof(My_Struct))" + , "if (tmp == nullptr) {" + , " return false;" + , "}" + , "ob->mem = tmp;" + , "```" + , "" + , "**Reason:** This avoids accidentally putting `nullptr` into a location without" + , "checking first. Putting `nullptr` somewhere may be ok, but we must do it" + , "intentionally." + ])) diff --git a/src/Tokstyle/Linter/MallocType.hs b/src/Tokstyle/Linter/MallocType.hs index 6b8e40a..b14521d 100644 --- a/src/Tokstyle/Linter/MallocType.hs +++ b/src/Tokstyle/Linter/MallocType.hs @@ -14,11 +14,14 @@ import Language.Cimple.Diagnostics (warn) import Language.Cimple.Pretty (showNode) import Language.Cimple.TraverseAst (AstActions, astActions, doNode, traverseAst) -import Tokstyle.Common (semEq) +import Tokstyle.Common (semEq, skip) supportedTypes :: [Text] supportedTypes = ["char", "uint8_t", "int16_t"] +mallocs :: [Text] +mallocs = ["mem_balloc", "malloc"] + isByteSize :: Node (Lexeme Text) -> Bool isByteSize ty = case unFix ty of TyStd (L _ _ "char") -> True @@ -26,26 +29,43 @@ isByteSize ty = case unFix ty of TyStd (L _ _ "uint8_t") -> True _ -> False -checkType :: FilePath -> Node (Lexeme Text) -> State [Text] () -checkType file castTy = case unFix castTy of +checkType :: FilePath -> Text -> Node (Lexeme Text) -> State [Text] () +checkType file malloc castTy = case unFix castTy of TyPointer (Fix (TyStd (L _ _ tyName))) | tyName `elem` supportedTypes -> return () _ -> warn file castTy $ - "`mem_balloc` should be used for builtin types only " - <> "(e.g. `uint8_t *` or `int16_t *`); use `calloc` instead" + "`" <> malloc <> "` should be used for builtin types only " + <> "(e.g. `uint8_t *` or `int16_t *`); use `mem_alloc` instead" + +checkExpr :: FilePath -> Text -> Node (Lexeme Text) -> State [Text] () +checkExpr file malloc size = case unFix size of + SizeofType{} -> + warn file size $ "`sizeof` in call to `" <> malloc <> "` should appear only once, " + <> "and only on the right hand side of the expression" + BinaryExpr l _ r -> do + checkExpr file malloc l + checkExpr file malloc r + VarExpr{} -> return () + LiteralExpr{} -> return () + MemberAccess e _ -> checkExpr file malloc e + PointerAccess e _ -> checkExpr file malloc e + x -> + warn file size $ "`" <> malloc <> "` should only have sizeof and simple expression arguments: " <> Text.pack (show x) -checkSize :: FilePath -> Node (Lexeme Text) -> Node (Lexeme Text) -> State [Text] () -checkSize file castTy@(Fix (TyPointer objTy)) size = case unFix size of - BinaryExpr _ BopMul r -> checkSize file castTy r +checkSize :: FilePath -> Text -> Node (Lexeme Text) -> Node (Lexeme Text) -> State [Text] () +checkSize file malloc castTy@(Fix (TyPointer objTy)) size = case unFix size of + BinaryExpr l BopMul r -> do + checkExpr file malloc l + checkSize file malloc castTy r SizeofType sizeTy -> unless (sizeTy `semEq` objTy) $ - warn file size $ "`size` argument in call to `mem_balloc` indicates " + warn file size $ "`size` argument in call to `" <> malloc <> "` indicates " <> "creation of an array with element type `" <> showNode sizeTy <> "`, " <> "but result is cast to `" <> showNode castTy <> "`" _ -> unless (isByteSize objTy) $ - warn file size "`mem_balloc` result must be cast to a byte-sized type if `sizeof` is omitted" -checkSize file castTy _ = - warn file castTy "`mem_balloc` result must be cast to a pointer type" + warn file size $ "`" <> malloc <> "` result must be cast to a byte-sized type if `sizeof` is omitted" +checkSize file malloc castTy _ = + warn file castTy $ "`" <> malloc <> "` result must be cast to a pointer type" linter :: AstActions (State [Text]) Text @@ -55,18 +75,21 @@ linter = astActions -- Windows API weirdness: ignore completely. CastExpr (Fix (TyPointer (Fix (TyStd (L _ _ "IP_ADAPTER_INFO"))))) _ -> return () + CastExpr castTy (Fix (FunctionCall (Fix (VarExpr (L _ _ "malloc"))) [size])) -> do + checkType file "malloc" castTy + checkSize file "malloc" castTy size CastExpr castTy (Fix (FunctionCall (Fix (VarExpr (L _ _ "mem_balloc"))) [_, size])) -> do - checkType file castTy - checkSize file castTy size + checkType file "mem_balloc" castTy + checkSize file "mem_balloc" castTy size - FunctionCall (Fix (VarExpr (L _ _ "mem_balloc"))) _ -> - warn file node "the result of `mem_balloc` must be cast; plain `void *` is not supported" + FunctionCall (Fix (VarExpr (L _ _ name))) _ | name `elem` mallocs -> + warn file node $ "the result of `" <> name <> "` must be cast; plain `void *` is not supported" _ -> act } analyse :: (FilePath, [Node (Lexeme Text)]) -> [Text] -analyse = reverse . flip State.execState [] . traverseAst linter +analyse = reverse . flip State.execState [] . traverseAst linter . skip ["toxcore/mem.c"] descr :: ((FilePath, [Node (Lexeme Text)]) -> [Text], (Text, Text)) descr = (analyse, ("malloc-type", Text.unlines diff --git a/test/Tokstyle/Linter/MallocCallSpec.hs b/test/Tokstyle/Linter/MallocCallSpec.hs new file mode 100644 index 0000000..43923dd --- /dev/null +++ b/test/Tokstyle/Linter/MallocCallSpec.hs @@ -0,0 +1,53 @@ +{-# LANGUAGE OverloadedStrings #-} +module Tokstyle.Linter.MallocCallSpec where + +import Test.Hspec (Spec, it, shouldBe) + +import Tokstyle.Linter (allWarnings, analyseLocal) +import Tokstyle.LinterSpec (mustParse) + + +spec :: Spec +spec = do + it "warns when mem_alloc() is used outside a var decl stmt" $ do + ast <- mustParse + [ "int a(My_Struct **v) {" + , " *v = (My_Struct *)mem_alloc(mem, sizeof(My_Struct));" + , "}" + ] + analyseLocal allWarnings ("test.c", ast) + `shouldBe` + ["test.c:2: allocations using `mem_alloc` must first be assigned to a local variable or returned directly [-Wmalloc-call]"] + + it "warns if there is no null-check after an allocation" $ do + ast <- mustParse + [ "void a(My_Struct **v) {" + , " My_Struct *tmp = (My_Struct *)mem_alloc(mem, sizeof(My_Struct));" + , " *v = tmp;" + , "}" + ] + analyseLocal allWarnings ("test.c", ast) + `shouldBe` ["test.c:3: `tmp`, assigned from `mem_alloc` must immediately be checked against `nullptr` [-Wmalloc-call]"] + + it "accepts mem_alloc() being used in a var decl stmt" $ do + ast <- mustParse + [ "bool a(My_Struct **v) {" + , " My_Struct *tmp = (My_Struct *)mem_alloc(mem, sizeof(My_Struct));" + , " if (tmp == nullptr) {" + , " return false;" + , " }" + , " *v = tmp;" + , " return true;" + , "}" + ] + analyseLocal allWarnings ("test.c", ast) + `shouldBe` [] + + it "accepts mem_alloc() being used in a return statement" $ do + ast <- mustParse + [ "My_Struct *my_struct_new(void) {" + , " return (My_Struct *)mem_alloc(mem, sizeof(My_Struct));" + , "}" + ] + analyseLocal allWarnings ("test.c", ast) + `shouldBe` [] diff --git a/tokstyle.cabal b/tokstyle.cabal index 367d282..9fee7ca 100644 --- a/tokstyle.cabal +++ b/tokstyle.cabal @@ -53,6 +53,7 @@ library Tokstyle.Linter.LoggerCalls Tokstyle.Linter.LoggerConst Tokstyle.Linter.LoggerNoEscapes + Tokstyle.Linter.MallocCall Tokstyle.Linter.MallocType Tokstyle.Linter.MemcpyStructs Tokstyle.Linter.MissingNonNull