From abfb6df7415ca7626d4ef5029da17b7511dafafb Mon Sep 17 00:00:00 2001 From: iphydf Date: Sun, 25 Oct 2026 18:47:26 +0000 Subject: [PATCH] feat: Add `-Wwraparound` to check for index out of bounds. --- doc/c.md | 19 +- src/Tokstyle/C/Analysis/Interval.hs | 13 + src/Tokstyle/C/Linter.hs | 4 + src/Tokstyle/C/Linter/Wraparound.hs | 182 ++++++++++ stack.yaml | 2 +- test/Tokstyle/C/Linter/WraparoundSpec.hs | 405 +++++++++++++++++++++++ tokstyle.cabal | 2 + 7 files changed, 625 insertions(+), 2 deletions(-) create mode 100644 src/Tokstyle/C/Linter/Wraparound.hs create mode 100644 test/Tokstyle/C/Linter/WraparoundSpec.hs diff --git a/doc/c.md b/doc/c.md index bcffd35..3ae0061 100644 --- a/doc/c.md +++ b/doc/c.md @@ -1,6 +1,6 @@ # C-based linters (`check-c`) -There are currently 16 linters implemented. +There are currently 17 linters implemented. ## `-Wbool-conversion` @@ -101,3 +101,20 @@ role-based types. ## `-Wvoid-call` Checks that the first statement of a function with a `void*` parameter casts it to a specific type. + +## `-Wwraparound` + +Checks for integer arithmetic that wraps around where it is used as an index: +compared to an index reduced with `%` or `&`, reduced with `%` by a size that +doesn't divide the type's range, or used to index an array it can be past the +end of. Also checks for the difference of two 16-bit values that wraps around +at 32 bits because integer promotion computes it in `int`, and for such a +difference compared in `int`, where it is negative instead of wrapping around. + +The checks follow values through locals and fields, take the conditions that +guard them into account, and know which values the program stores in struct +fields, returns from functions, and passes to static functions. + +**Reason:** in a ring buffer of size `N`, `(uint16_t)(start - 1)` wraps to +65535 when `start` is 0, not to `N - 1`. Comparing it to `id % N` then never +matches. Use `(start + N - 1) % N` instead. diff --git a/src/Tokstyle/C/Analysis/Interval.hs b/src/Tokstyle/C/Analysis/Interval.hs index c6836e6..45321c0 100644 --- a/src/Tokstyle/C/Analysis/Interval.hs +++ b/src/Tokstyle/C/Analysis/Interval.hs @@ -38,6 +38,7 @@ module Tokstyle.C.Analysis.Interval , assume , effects , subTerms + , subTermsWith , normalise , convert , intType @@ -429,6 +430,18 @@ binaryType op a b subTerms :: Term -> [Term] subTerms t = t : concatMap subTerms (children t) +-- | Each term within a term, with the facts where it is evaluated. The right +-- operand of @&&@ or @||@ and the branches of @?:@ are only evaluated where +-- what comes before them has the value that leads to them. +subTermsWith :: Facts -> Term -> [(Facts, Term)] +subTermsWith facts t = (facts, t) : case termKind t of + TBin CLndOp a b -> subTermsWith facts a ++ after True a b + TBin CLorOp a b -> subTermsWith facts a ++ after False a b + TCond c m f -> subTermsWith facts c ++ maybe [] (after True c) m ++ after False c f + _ -> concatMap (subTermsWith facts) (children t) + where + after truth c x = maybe [] (`subTermsWith` x) (assume truth c facts) + ------------------------------------------------------------------------------ -- Evaluating terms diff --git a/src/Tokstyle/C/Linter.hs b/src/Tokstyle/C/Linter.hs index 6d476e4..86e80cd 100644 --- a/src/Tokstyle/C/Linter.hs +++ b/src/Tokstyle/C/Linter.hs @@ -76,6 +76,8 @@ import qualified Tokstyle.C.Linter.SizeArg as SizeArg (descr) import qualified Tokstyle.C.Linter.Sizeof as Sizeof (descr) import qualified Tokstyle.C.Linter.StrictTypedef as StrictTypedef (descr) import qualified Tokstyle.C.Linter.VoidCall as VoidCall (descr) +import qualified Tokstyle.C.Linter.Wraparound as Wraparound (descr, + looksAt) type Linter = (GlobalDecls -> Trav Env (), (Text, Text)) @@ -98,6 +100,7 @@ linters = , Sizeof.descr , StrictTypedef.descr , VoidCall.descr + , Wraparound.descr ] @@ -106,6 +109,7 @@ linters = intervalLinters :: [(Text, FunDef -> Bool)] intervalLinters = [ ("buffer-size", BufferSize.looksAt) + , ("wraparound", Wraparound.looksAt) ] diff --git a/src/Tokstyle/C/Linter/Wraparound.hs b/src/Tokstyle/C/Linter/Wraparound.hs new file mode 100644 index 0000000..a1d76ee --- /dev/null +++ b/src/Tokstyle/C/Linter/Wraparound.hs @@ -0,0 +1,182 @@ +{-# LANGUAGE LambdaCase #-} +{-# LANGUAGE OverloadedStrings #-} +{-# LANGUAGE Strict #-} +module Tokstyle.C.Linter.Wraparound (descr, looksAt) where + +import Control.Monad (forM_, when) +import Data.Text (Text) +import qualified Data.Text as Text +import Language.C.Analysis.SemRep (FunDef, GlobalDecls) +import Language.C.Analysis.TravMonad (Trav) +import Language.C.Data.Node (NodeInfo) +import Language.C.Syntax.AST (CAssignOp (..), CBinaryOp (..), + annotation) +import Prettyprinter (pretty, (<+>)) +import Tokstyle.C.Analysis.Interval (Facts, IntTy (..), Origin (..), + Range (..), Term (..), + TermKind (..), canWrap, eval, + subTermsWith) +import Tokstyle.C.Env (Env, recordLinterError) +import Tokstyle.C.IntervalLinter (analysedFunctions, exprDoc, + recordWrap, valuesDoc) +import Tokstyle.C.TravUtils (backticks) + + +isComparison :: CBinaryOp -> Bool +isComparison = (`elem` [CEqOp, CNeqOp, CLeOp, CGrOp, CLeqOp, CGeqOp]) + +isRelational :: CBinaryOp -> Bool +isRelational = (`elem` [CLeOp, CGrOp, CLeqOp, CGeqOp]) + +compareWith :: CBinaryOp -> Integer -> Integer -> Bool +compareWith = \case + CEqOp -> (==) + CNeqOp -> (/=) + CLeOp -> (<) + CGrOp -> (>) + CLeqOp -> (<=) + _ -> (>=) + +flipOp :: CBinaryOp -> CBinaryOp +flipOp = \case + CLeOp -> CGrOp + CGrOp -> CLeOp + CLeqOp -> CGeqOp + CGeqOp -> CLeqOp + op -> op + +-- | The values that wrapped around, with the expression where they did. +wrappedIn :: Facts -> Term -> [(Range, NodeInfo)] +wrappedIn facts t = [ (r, site) | Just rs <- [eval facts t], r@(Range _ _ (Wrapped site)) <- rs ] + +-- | Warns if wrapping around makes one side of a comparison take values an +-- index reduced on the other side never has. +checkComparison :: Facts -> CBinaryOp -> Term -> Term -> Trav Env () +checkComparison facts op wrapping other = case eval facts other of + Just os@(_ : _) | all ((== Reduced) . origin) os -> do + let oLo = minimum (map lo os) + oHi = maximum (map hi os) + case [ (w, site) | (w, site) <- wrappedIn facts wrapping, hi w < oLo || lo w > oHi ] of + (w, site) : _ -> recordWrap wrapping site $ + exprDoc (termExpr wrapping) <+> "can wrap around to" <+> valuesDoc w + <> ", but" <+> exprDoc (termExpr other) <+> "is always between" + <+> pretty oLo <+> "and" <+> pretty oHi + <> ", so the comparison is always" + <+> (if compareWith op (lo w) oLo then "true" else "false") + <+> "when it wraps" + [] -> return () + _ -> return () + +-- | Warns if a value that wrapped around is reduced modulo a size that +-- doesn't divide the range it wrapped around. +checkRemainder :: Facts -> Term -> Term -> Trav Env () +checkRemainder facts a b = case (eval facts b, termTy a, wrappedIn facts a) of + (Just [Range n n' _], Just ty, (w, site) : _) | n == n', n > 0, (2 ^ tyBits ty) `mod` n /= 0 -> + recordWrap a site $ + exprDoc (termExpr a) <+> "can wrap around to" <+> valuesDoc w + <> ", and" <+> pretty (2 ^ tyBits ty :: Integer) <+> "is not a multiple of" + <+> pretty n <> ", so" <+> backticks ("%" <+> pretty n) + <+> "then gives a different index than it would without the wrap" + _ -> return () + +-- | Warns if an index that wrapped around can be past the end of an array. +checkIndex :: Facts -> Term -> Term -> Integer -> Trav Env () +checkIndex facts arr i len = + case [ (w, site) | (w, site) <- wrappedIn facts i, hi w >= len ] of + (w, site) : _ -> recordWrap i site $ + exprDoc (termExpr i) <+> "can wrap around to" <+> valuesDoc w + <> ", past the end of" <+> exprDoc (termExpr arr) + <> ", which has" <+> pretty len <+> "elements" + [] -> return () + +-- | Warns if the difference of two narrow unsigned values, computed in @int@ +-- after integer promotion, wraps around at the size of the wider unsigned type +-- it is converted to. +checkPromoted :: Facts -> Term -> Term -> Trav Env () +checkPromoted facts converted inner = case (termTy converted, termKind inner, termTy inner) of + (Just IntTy{tySigned = False, tyBits = bits}, TBin CSubOp x y, Just IntTy{tySigned = True}) + | Just w <- narrowest [x, y], w < bits + , (r, _) : _ <- wrappedIn facts converted -> + recordLinterError (annotation (termExpr inner)) $ + exprDoc (termExpr inner) <+> "can wrap around to" <+> valuesDoc r + <> ", because it is the difference of" <+> pretty w <> "-bit values but wraps around at" + <+> pretty bits <+> "bits here" + _ -> return () + +-- | The width of the widest of some narrow unsigned values, if none of them is +-- a constant. +narrowest :: [Term] -> Maybe Integer +narrowest ts = maximum <$> mapM width ts + where + width t = case (termKind t, termTy t) of + (TConst _, _) -> Nothing + (_, Just IntTy{tySigned = False, tyBits = b}) -> Just b + _ -> Nothing + +-- | The width of the narrow unsigned values a term is the difference of, if +-- integer promotion computes it in @int@ and it can be negative there. +negativeDifference :: Facts -> Term -> Maybe Integer +negativeDifference facts d = case (termKind d, termTy d) of + (TBin CSubOp x y, Just IntTy{tySigned = True, tyBits = bits}) + | Just w <- narrowest [x, y], w < bits + , Just rs <- eval facts d, any ((< 0) . lo) rs -> Just w + _ -> Nothing + +-- | Warns if a difference of narrow unsigned values that is negative where it +-- would wrap around at their width is compared with another value that isn't +-- known. +checkSignedComparison :: Facts -> Term -> Term -> Trav Env () +checkSignedComparison facts l r = + case [ (d, other, w) | (d, other) <- [(l, r), (r, l)], Just w <- [negativeDifference facts d] ] of + (d, other, w) : _ | not (known other) -> + recordLinterError (annotation (termExpr d)) $ + exprDoc (termExpr d) <+> "is the difference of" <+> pretty w + <> "-bit values, computed in `int` where it can be negative instead of wrapping around at" + <+> pretty w <+> "bits, so comparing it with" <+> exprDoc (termExpr other) + <+> "gives a different result than comparing the" <+> pretty w <> "-bit differences" + _ -> return () + where + known t = case eval facts t of + Just [Range n n' _] -> n == n' + _ -> False + +checkTerm :: Facts -> Term -> Trav Env () +checkTerm facts t = case termKind t of + TBin op l r | isComparison op -> do + checkComparison facts op l r + checkComparison facts (flipOp op) r l + when (isRelational op) $ checkSignedComparison facts l r + TBin CRmdOp a b -> checkRemainder facts a b + TIndex arr i (Just len) -> checkIndex facts arr i len + TCast a -> checkPromoted facts t a + TAssign _ CAssignOp _ r -> checkPromoted facts t r + _ -> return () + +-- | The functions the check looks at: those that can wrap around. +looksAt :: FunDef -> Bool +looksAt = canWrap + +analyse :: GlobalDecls -> Trav Env () +analyse _ = do + functions <- analysedFunctions looksAt + forM_ functions $ \(_, nodes) -> + forM_ nodes $ \(_, facts, terms) -> mapM_ (uncurry checkTerm) (concatMap (subTermsWith facts) terms) + + +descr :: (GlobalDecls -> Trav Env (), (Text, Text)) +descr = (analyse, ("wraparound", Text.unlines + [ "Checks for integer arithmetic that wraps around where it is used as an index:" + , "compared to an index reduced with `%` or `&`, reduced with `%` by a size that" + , "doesn't divide the type's range, or used to index an array it can be past the" + , "end of. Also checks for the difference of two 16-bit values that wraps around" + , "at 32 bits because integer promotion computes it in `int`, and for such a" + , "difference compared in `int`, where it is negative instead of wrapping around." + , "" + , "The checks follow values through locals and fields, take the conditions that" + , "guard them into account, and know which values the program stores in struct" + , "fields, returns from functions, and passes to static functions." + , "" + , "**Reason:** in a ring buffer of size `N`, `(uint16_t)(start - 1)` wraps to" + , "65535 when `start` is 0, not to `N - 1`. Comparing it to `id % N` then never" + , "matches. Use `(start + N - 1) % N` instead." + ])) diff --git a/stack.yaml b/stack.yaml index def6dff..48e4af9 100644 --- a/stack.yaml +++ b/stack.yaml @@ -1,5 +1,5 @@ --- packages: [.] -resolver: lts-21.9 +resolver: lts-23.9 extra-deps: - cimple-0.0.32 diff --git a/test/Tokstyle/C/Linter/WraparoundSpec.hs b/test/Tokstyle/C/Linter/WraparoundSpec.hs new file mode 100644 index 0000000..545bed5 --- /dev/null +++ b/test/Tokstyle/C/Linter/WraparoundSpec.hs @@ -0,0 +1,405 @@ +{-# LANGUAGE OverloadedStrings #-} +module Tokstyle.C.Linter.WraparoundSpec (spec) where +import Data.Text (Text) +import Test.Hspec (Spec, it) + +import Tokstyle.C.LinterSpec (shouldAccept, shouldWarn) + + +shouldWarn' :: [Text] -> [[Text]] -> IO () +shouldWarn' = shouldWarn ["wraparound"] + + +shouldAccept' :: [Text] -> IO () +shouldAccept' = shouldAccept ["wraparound"] + + +prelude :: [Text] +prelude = + [ "typedef unsigned char uint8_t;" + , "typedef unsigned short uint16_t;" + , "typedef unsigned int uint32_t;" + , "typedef struct Conn { uint16_t start; uint32_t id; uint8_t slots[2048]; } Conn;" + ] + + +spec :: Spec +spec = do + it "warns about a ring index decremented modulo the wrong size" $ do + shouldWarn' (prelude ++ + [ "int full(const Conn *c) {" + , " return (c->id % 2048) == (uint16_t)(c->start - 1);" + , "}" + ]) + [[ "error: `(uint16_t) (c->start - 1)` can wrap around to 65535, but `c->id % 2048` is always between 0 and 2047, so the comparison is always false when it wraps [-Wwraparound]" + , " --> test.c:6:28" + , " |" + , " 6| return (c->id % 2048) == (uint16_t)(c->start - 1);" + , " | ^^^^^^^^^^^^^^^^^^^^^^^^" + ]] + + it "warns about unsigned subtraction without a cast" $ do + shouldWarn' (prelude ++ + [ "int full(uint32_t id, uint32_t start) {" + , " return start - 1 != id % 2048;" + , "}" + ]) + [[ "error: `start - 1` can wrap around to 4294967295, but `id % 2048` is always between 0 and 2047, so the comparison is always true when it wraps [-Wwraparound]" + , " --> test.c:6:10" + , " |" + , " 6| return start - 1 != id % 2048;" + , " | ^^^^^^^^^" + ]] + + it "follows wrapped values through locals" $ do + shouldWarn' (prelude ++ + [ "int full(const Conn *c) {" + , " const uint16_t prev = c->start - 1;" + , " const uint32_t idx = c->id % 2048;" + , " return idx == prev;" + , "}" + ]) + [[ "error: `prev` can wrap around to 65535, but `idx` is always between 0 and 2047, so the comparison is always false when it wraps [-Wwraparound]" + , " --> test.c:8:17" + , " |" + , " 6| const uint16_t prev = c->start - 1;" + , " | ^^^^^^^^^^^^" + , " | |" + , " | wraps around here" + , " 8| return idx == prev;" + , " | ^^^^" + ]] + + it "warns about a wrapped value reduced modulo a size that doesn't divide its range" $ do + shouldWarn' (prelude ++ + [ "int used(const uint8_t *relays, uint16_t counter) {" + , " int n = 0;" + , " for (unsigned i = 0; i < 24; ++i) {" + , " n += relays[(counter - (i + 1)) % (6 * 4)];" + , " }" + , " return n;" + , "}" + ]) + [[ "error: `counter - (i + 1)` can wrap around to values from 4294967272 to 4294967295, and 4294967296 is not a multiple of 24, so `% 24` then gives a different index than it would without the wrap [-Wwraparound]" + , " --> test.c:8:18" + , " |" + , " 8| n += relays[(counter - (i + 1)) % (6 * 4)];" + , " | ^^^^^^^^^^^^^^^^^" + ]] + + it "warns about a wrapped index past the end of an array" $ do + shouldWarn' (prelude ++ + [ "uint8_t last(const Conn *c) {" + , " return c->slots[(uint16_t)(c->start - 1)];" + , "}" + ]) + [[ "error: `(uint16_t) (c->start - 1)` can wrap around to 65535, past the end of `c->slots`, which has 2048 elements [-Wwraparound]" + , " --> test.c:6:19" + , " |" + , " 6| return c->slots[(uint16_t)(c->start - 1)];" + , " | ^^^^^^^^^^^^^^^^^^^^^^^^" + ]] + + it "forgets facts about fields when a call can change them" $ do + shouldWarn' (prelude ++ + [ "void reset(Conn *c);" + , "int full(Conn *c) {" + , " if (c->start == 0) {" + , " return 0;" + , " }" + , " reset(c);" + , " return (c->id % 2048) == (uint16_t)(c->start - 1);" + , "}" + ]) + [[ "error: `(uint16_t) (c->start - 1)` can wrap around to 65535, but `c->id % 2048` is always between 0 and 2047, so the comparison is always false when it wraps [-Wwraparound]" + , " --> test.c:11:28" + , " |" + , " 11| return (c->id % 2048) == (uint16_t)(c->start - 1);" + , " | ^^^^^^^^^^^^^^^^^^^^^^^^" + ]] + + it "doesn't assume a static local still has its initial value" $ do + shouldWarn' (prelude ++ + [ "int full(const Conn *c) {" + , " static uint16_t start = 1;" + , " const int r = (c->id % 2048) == (uint16_t)(start - 1);" + , " start = c->start;" + , " return r;" + , "}" + ]) + [[ "error: `(uint16_t) (start - 1)` can wrap around to 65535, but `c->id % 2048` is always between 0 and 2047, so the comparison is always false when it wraps [-Wwraparound]" + , " --> test.c:7:35" + , " |" + , " 7| const int r = (c->id % 2048) == (uint16_t)(start - 1);" + , " | ^^^^^^^^^^^^^^^^^^^^^" + ]] + + it "warns about the difference of 16-bit values wrapping around at 32 bits" $ do + shouldWarn' (prelude ++ + [ "int late(uint16_t seq, uint16_t bottom, uint32_t size) {" + , " return (uint32_t)(seq - bottom) > size;" + , "}" + ]) + [[ "error: `seq - bottom` can wrap around to values from 4294901761 to 4294967295, because it is the difference of 16-bit values but wraps around at 32 bits here [-Wwraparound]" + , " --> test.c:6:21" + , " |" + , " 6| return (uint32_t)(seq - bottom) > size;" + , " | ^^^^^^^^^^^^" + ]] + + it "accepts the difference of 16-bit values wrapping around at 16 bits" $ do + shouldAccept' (prelude ++ + [ "int late(uint16_t seq, uint16_t bottom, uint32_t size) {" + , " return (uint16_t)(seq - bottom) > size;" + , "}" + ]) + + it "warns about differences of 16-bit values compared in int" $ do + shouldWarn' (prelude ++ + [ "int newer(uint16_t seq, uint16_t bottom, uint16_t top) {" + , " return (seq - bottom) >= (top - bottom);" + , "}" + ]) + [[ "error: `seq - bottom` is the difference of 16-bit values, computed in `int` where it can be negative instead of wrapping around at 16 bits, so comparing it with `top - bottom` gives a different result than comparing the 16-bit differences [-Wwraparound]" + , " --> test.c:6:11" + , " |" + , " 6| return (seq - bottom) >= (top - bottom);" + , " | ^^^^^^^^^^^^" + ]] + + it "accepts differences of 16-bit values compared at 16 bits" $ do + shouldAccept' (prelude ++ + [ "int newer(uint16_t seq, uint16_t bottom, uint16_t top) {" + , " return (uint16_t)(seq - bottom) >= (uint16_t)(top - bottom);" + , "}" + ]) + + it "accepts a difference of 16-bit values compared with a known value" $ do + shouldAccept' (prelude ++ + [ "int before(uint16_t a, uint16_t b) {" + , " return a - b < 0;" + , "}" + ]) + + it "accepts differences of 16-bit values that can't be negative" $ do + shouldAccept' (prelude ++ + [ "int newer(uint16_t seq, uint16_t bottom, uint16_t top) {" + , " if (seq < bottom) {" + , " return 0;" + , " }" + , " if (top < bottom) {" + , " return 0;" + , " }" + , " return (seq - bottom) >= (top - bottom);" + , "}" + ]) + + it "knows what the left side of || rules out on its right side" $ do + shouldAccept' (prelude ++ + [ "int fits(uint16_t length, uint16_t offset, uint16_t size) {" + , " return length <= offset || length - offset < size - 80;" + , "}" + ]) + + it "knows a field holds an index another function reduces" $ do + shouldWarn' (prelude ++ + [ "void advance(Conn *c) {" + , " c->start = (c->start + 1) % 2048;" + , "}" + , "int full(const Conn *c, uint16_t id) {" + , " return c->start == (uint16_t)(id - 1);" + , "}" + ]) + [[ "error: `(uint16_t) (id - 1)` can wrap around to 65535, but `c->start` is always between 0 and 2047, so the comparison is always false when it wraps [-Wwraparound]" + , " --> test.c:9:22" + , " |" + , " 9| return c->start == (uint16_t)(id - 1);" + , " | ^^^^^^^^^^^^^^^^^^" + ]] + + it "forgets what a field holds once its address escapes" $ do + shouldAccept' (prelude ++ + [ "void load(uint16_t *value);" + , "void advance(Conn *c) {" + , " c->start = (c->start + 1) % 2048;" + , " load(&c->start);" + , "}" + , "int full(const Conn *c, uint16_t id) {" + , " return c->start == (uint16_t)(id - 1);" + , "}" + ]) + + it "forgets what a struct's fields hold once a function it doesn't see can write them" $ do + shouldAccept' (prelude ++ + [ "void load(Conn *c);" + , "void advance(Conn *c) {" + , " c->start = (c->start + 1) % 2048;" + , " load(c);" + , "}" + , "int full(const Conn *c, uint16_t id) {" + , " return c->start == (uint16_t)(id - 1);" + , "}" + ]) + + it "knows what a function returns" $ do + shouldWarn' (prelude ++ + [ "static uint16_t index_of(uint32_t id) {" + , " return id % 2048;" + , "}" + , "int full(uint32_t id, uint16_t start) {" + , " return index_of(id) == (uint16_t)(start - 1);" + , "}" + ]) + [[ "error: `(uint16_t) (start - 1)` can wrap around to 65535, but `index_of(id)` is always between 0 and 2047, so the comparison is always false when it wraps [-Wwraparound]" + , " --> test.c:9:26" + , " |" + , " 9| return index_of(id) == (uint16_t)(start - 1);" + , " | ^^^^^^^^^^^^^^^^^^^^^" + ]] + + it "knows what a static function's callers pass" $ do + shouldAccept' (prelude ++ + [ "static uint8_t before(const Conn *c, uint16_t i) {" + , " return c->slots[(uint16_t)(i - 1)];" + , "}" + , "uint8_t first(const Conn *c) {" + , " return before(c, 1);" + , "}" + ]) + + it "doesn't assume what callers pass to a function whose address escapes" $ do + shouldWarn' (prelude ++ + [ "static uint8_t before(const Conn *c, uint16_t i) {" + , " return c->slots[(uint16_t)(i - 1)];" + , "}" + , "uint8_t (*const get)(const Conn *c, uint16_t i) = before;" + , "uint8_t first(const Conn *c) {" + , " return before(c, 1);" + , "}" + ]) + [[ "error: `(uint16_t) (i - 1)` can wrap around to 65535, past the end of `c->slots`, which has 2048 elements [-Wwraparound]" + , " --> test.c:6:19" + , " |" + , " 6| return c->slots[(uint16_t)(i - 1)];" + , " | ^^^^^^^^^^^^^^^^^" + ]] + + it "knows a difference can't wrap after a guard relating its operands" $ do + shouldAccept' (prelude ++ + [ "uint8_t rest(const Conn *c, uint16_t size, uint16_t offset) {" + , " if (size < offset) {" + , " return 0;" + , " }" + , " return c->slots[(uint16_t)(size - offset)];" + , "}" + ]) + + it "knows a loop bound is past the index" $ do + shouldAccept' (prelude ++ + [ "uint8_t last(const Conn *c, uint16_t count) {" + , " uint8_t x = 0;" + , " for (uint16_t i = 0; i < count; ++i) {" + , " x = c->slots[(uint16_t)(count - i - 1)];" + , " }" + , " return x;" + , "}" + ]) + + it "forgets how paths relate after writing one of them" $ do + shouldWarn' (prelude ++ + [ "uint8_t rest(const Conn *c, uint16_t size, uint16_t offset, uint16_t other) {" + , " if (size < offset) {" + , " return 0;" + , " }" + , " offset = other;" + , " return c->slots[(uint16_t)(size - offset)];" + , "}" + ]) + [[ "error: `(uint16_t) (size - offset)` can wrap around to values from 1 to 65535, past the end of `c->slots`, which has 2048 elements [-Wwraparound]" + , " --> test.c:10:19" + , " |" + , " 10| return c->slots[(uint16_t)(size - offset)];" + , " | ^^^^^^^^^^^^^^^^^^^^^^^^^" + ]] + + it "accepts a decrement modulo the ring size" $ do + shouldAccept' (prelude ++ + [ "int full(const Conn *c) {" + , " return (c->id % 2048) == (c->start + 2048 - 1) % 2048;" + , "}" + ]) + + it "accepts a wrapped value reduced modulo a divisor of its range" $ do + shouldAccept' (prelude ++ + [ "int full(const Conn *c) {" + , " return (c->id % 2048) == (uint16_t)(c->start - 1) % 2048;" + , "}" + ]) + + it "accepts a decrement guarded against wrapping" $ do + shouldAccept' (prelude ++ + [ "int full(const Conn *c) {" + , " if (c->start == 0) {" + , " return 0;" + , " }" + , " return (c->id % 2048) == (uint16_t)(c->start - 1);" + , "}" + ]) + + it "accepts a decrement in a switch case that excludes wrapping" $ do + shouldAccept' (prelude ++ + [ "int full(const Conn *c) {" + , " switch (c->start) {" + , " case 0:" + , " return 0;" + , " default:" + , " return (c->id % 2048) == (uint16_t)(c->start - 1);" + , " }" + , "}" + ]) + + it "accepts a decrement in a loop that stops before wrapping" $ do + shouldAccept' (prelude ++ + [ "int find(const Conn *c, uint32_t n) {" + , " while (n > 0) {" + , " if ((c->id % 2048) == n - 1) {" + , " return 1;" + , " }" + , " --n;" + , " }" + , " return 0;" + , "}" + ]) + + it "checks the first iteration of a do-while loop before its condition" $ do + shouldWarn' (prelude ++ + [ "int find(const Conn *c, uint32_t n) {" + , " do {" + , " if ((c->id % 2048) == n - 1) {" + , " return 1;" + , " }" + , " } while (--n > 0);" + , " return 0;" + , "}" + ]) + [[ "error: `n - 1` can wrap around to 4294967295, but `c->id % 2048` is always between 0 and 2047, so the comparison is always false when it wraps [-Wwraparound]" + , " --> test.c:7:27" + , " |" + , " 7| if ((c->id % 2048) == n - 1) {" + , " | ^^^^^" + ]] + + it "accepts the unsigned range check idiom" $ do + shouldAccept' (prelude ++ + [ "extern const uint16_t UINT16_MAX;" + , "int fits(uint32_t length) {" + , " return length - 1 < UINT16_MAX;" + , "}" + ]) + + it "accepts comparisons where the other side can have any value" $ do + shouldAccept' (prelude ++ + [ "int next(uint16_t a, uint16_t b) {" + , " return a == (uint16_t)(b - 1);" + , "}" + ]) diff --git a/tokstyle.cabal b/tokstyle.cabal index 39649dc..1f3fe0c 100644 --- a/tokstyle.cabal +++ b/tokstyle.cabal @@ -53,6 +53,7 @@ library Tokstyle.C.Linter.Sizeof Tokstyle.C.Linter.StrictTypedef Tokstyle.C.Linter.VoidCall + Tokstyle.C.Linter.Wraparound Tokstyle.C.Patterns Tokstyle.C.TravUtils Tokstyle.C.TraverseAst @@ -210,6 +211,7 @@ test-suite testsuite Tokstyle.C.Linter.SizeofSpec Tokstyle.C.Linter.StrictTypedefSpec Tokstyle.C.Linter.VoidCallSpec + Tokstyle.C.Linter.WraparoundSpec Tokstyle.C.LinterSpec Tokstyle.Linter.AssertSpec Tokstyle.Linter.BooleanReturnSpec