Skip to content

Commit

Permalink
feat: Add -Wmalloc-call.
Browse files Browse the repository at this point in the history
Checks that allocation functions like `mem_balloc` are always first
assigned to a local variable.
  • Loading branch information
iphydf committed Jan 8, 2024
1 parent 636ca43 commit eef8e09
Show file tree
Hide file tree
Showing 10 changed files with 295 additions and 24 deletions.
30 changes: 29 additions & 1 deletion doc/cimple.md
Original file line number Diff line number Diff line change
@@ -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`
Expand Down Expand Up @@ -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
Expand Down
7 changes: 7 additions & 0 deletions doc/generate
Original file line number Diff line number Diff line change
@@ -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"
12 changes: 12 additions & 0 deletions src/Tokstyle/Common.hs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
2 changes: 2 additions & 0 deletions src/Tokstyle/Linter.hs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -83,6 +84,7 @@ localLinters =
, LoggerCalls.descr
, LoggerConst.descr
, LoggerNoEscapes.descr
, MallocCall.descr
, MallocType.descr
, MemcpyStructs.descr
, MissingNonNull.descr
Expand Down
25 changes: 20 additions & 5 deletions src/Tokstyle/Linter/CallocArgs.hs
Original file line number Diff line number Diff line change
Expand Up @@ -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] ()
Expand All @@ -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
Expand All @@ -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
Expand All @@ -49,15 +58,21 @@ 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"

_ -> act
}

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
Expand Down
15 changes: 14 additions & 1 deletion src/Tokstyle/Linter/CallocType.hs
Original file line number Diff line number Diff line change
Expand Up @@ -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)


Expand All @@ -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
Expand All @@ -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)])) ->
Expand All @@ -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
Expand Down
117 changes: 117 additions & 0 deletions src/Tokstyle/Linter/MallocCall.hs
Original file line number Diff line number Diff line change
@@ -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."
]))
Loading

0 comments on commit eef8e09

Please sign in to comment.