From 1bff8fb0222ffe6ac77616bfe1a713aa7ec70f6d Mon Sep 17 00:00:00 2001 From: Stanimir Bozhilov Date: Sat, 8 Jul 2023 10:05:40 +0200 Subject: [PATCH 01/10] Add InvalidMemoryDeallocation message category --- src/util/messageCategory.ml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/util/messageCategory.ml b/src/util/messageCategory.ml index ddf91dba0b..5452225c26 100644 --- a/src/util/messageCategory.ml +++ b/src/util/messageCategory.ml @@ -12,6 +12,7 @@ type undefined_behavior = | NullPointerDereference | UseAfterFree | DoubleFree + | InvalidMemoryDeallocation | Uninitialized | DoubleLocking | Other @@ -65,6 +66,7 @@ struct let nullpointer_dereference: category = create @@ NullPointerDereference let use_after_free: category = create @@ UseAfterFree let double_free: category = create @@ DoubleFree + let invalid_memory_deallocation: category = create @@ InvalidMemoryDeallocation let uninitialized: category = create @@ Uninitialized let double_locking: category = create @@ DoubleLocking let other: category = create @@ Other @@ -102,6 +104,7 @@ struct | "nullpointer_dereference" -> nullpointer_dereference | "use_after_free" -> use_after_free | "double_free" -> double_free + | "invalid_memory_deallocation" -> invalid_memory_deallocation | "uninitialized" -> uninitialized | "double_locking" -> double_locking | "other" -> other @@ -113,6 +116,7 @@ struct | NullPointerDereference -> ["NullPointerDereference"] | UseAfterFree -> ["UseAfterFree"] | DoubleFree -> ["DoubleFree"] + | InvalidMemoryDeallocation -> ["InvalidMemoryDeallocation"] | Uninitialized -> ["Uninitialized"] | DoubleLocking -> ["DoubleLocking"] | Other -> ["Other"] @@ -223,6 +227,7 @@ let behaviorName = function |NullPointerDereference -> "NullPointerDereference" |UseAfterFree -> "UseAfterFree" |DoubleFree -> "DoubleFree" + |InvalidMemoryDeallocation -> "InvalidMemoryDeallocation" |Uninitialized -> "Uninitialized" |DoubleLocking -> "DoubleLocking" |Other -> "Other" From f8e8116460f9d8d5b8e91c2aa4b2390d6591477f Mon Sep 17 00:00:00 2001 From: Stanimir Bozhilov Date: Sat, 8 Jul 2023 10:06:02 +0200 Subject: [PATCH 02/10] Add checks for deallocation of non-dynamically allocated memory --- src/analyses/base.ml | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/analyses/base.ml b/src/analyses/base.ml index 72cc6a614f..cdc8e5a7d3 100644 --- a/src/analyses/base.ml +++ b/src/analyses/base.ml @@ -1997,6 +1997,20 @@ struct let st' = invalidate ~deep:false ~ctx (Analyses.ask_of_ctx ctx) gs st shallow_addrs in invalidate ~deep:true ~ctx (Analyses.ask_of_ctx ctx) gs st' deep_addrs + let check_free_of_non_heap_mem ctx special_fn ptr = + match ctx.ask (Queries.MayPointTo ptr) with + | a when not (Queries.LS.is_top a) -> + let warn_if_not_heap_var special_fn var = + if not (ctx.ask (Queries.IsHeapVar var)) then + M.warn ~category:(Behavior (Undefined InvalidMemoryDeallocation)) ~tags:[CWE 590] "Free of non-dynamically allocated memory in function %s for pointer %a" special_fn.vname d_exp ptr + in + let pointed_to_vars = + Queries.LS.elements a + |> List.map fst + in + List.iter (warn_if_not_heap_var special_fn) pointed_to_vars + | _ -> () + let special ctx (lv:lval option) (f: varinfo) (args: exp list) = let invalidate_ret_lv st = match lv with | Some lv -> @@ -2277,6 +2291,8 @@ struct | _ -> st end | Realloc { ptr = p; size }, _ -> + (* Realloc shouldn't be passed non-dynamically allocated memory *) + check_free_of_non_heap_mem ctx f p; begin match lv with | Some lv -> let ask = Analyses.ask_of_ctx ctx in @@ -2308,6 +2324,10 @@ struct | None -> st end + | Free ptr, _ -> + (* Free shouldn't be passed non-dynamically allocated memory *) + check_free_of_non_heap_mem ctx f ptr; + st | Assert { exp; refine; _ }, _ -> assert_fn ctx exp refine | Setjmp { env }, _ -> let ask = Analyses.ask_of_ctx ctx in From 25462b84432580f0a703a2d0a898eeb60f9a205c Mon Sep 17 00:00:00 2001 From: Stanimir Bozhilov Date: Sat, 8 Jul 2023 10:06:26 +0200 Subject: [PATCH 03/10] Add regression tests for invalid memory deallocation --- .../01-invalid-dealloc-simple.c | 14 +++++++++++ .../02-invalid-dealloc-struct.c | 14 +++++++++++ .../03-invalid-dealloc-array.c | 25 +++++++++++++++++++ .../75-invalid_dealloc/04-invalid-realloc.c | 25 +++++++++++++++++++ 4 files changed, 78 insertions(+) create mode 100644 tests/regression/75-invalid_dealloc/01-invalid-dealloc-simple.c create mode 100644 tests/regression/75-invalid_dealloc/02-invalid-dealloc-struct.c create mode 100644 tests/regression/75-invalid_dealloc/03-invalid-dealloc-array.c create mode 100644 tests/regression/75-invalid_dealloc/04-invalid-realloc.c diff --git a/tests/regression/75-invalid_dealloc/01-invalid-dealloc-simple.c b/tests/regression/75-invalid_dealloc/01-invalid-dealloc-simple.c new file mode 100644 index 0000000000..16fbd593f4 --- /dev/null +++ b/tests/regression/75-invalid_dealloc/01-invalid-dealloc-simple.c @@ -0,0 +1,14 @@ +#include + +int main(int argc, char const *argv[]) +{ + int a; + int *p = &a; + free(p); //WARN + + char b = 'b'; + char *p2 = &b; + free(p2); //WARN + + return 0; +} diff --git a/tests/regression/75-invalid_dealloc/02-invalid-dealloc-struct.c b/tests/regression/75-invalid_dealloc/02-invalid-dealloc-struct.c new file mode 100644 index 0000000000..6768103976 --- /dev/null +++ b/tests/regression/75-invalid_dealloc/02-invalid-dealloc-struct.c @@ -0,0 +1,14 @@ +#include + +typedef struct custom_t { + int x; + int y; +} custom_t; + +int main(int argc, char const *argv[]) +{ + custom_t *var; + free(var); //WARN + + return 0; +} diff --git a/tests/regression/75-invalid_dealloc/03-invalid-dealloc-array.c b/tests/regression/75-invalid_dealloc/03-invalid-dealloc-array.c new file mode 100644 index 0000000000..c023b5fc53 --- /dev/null +++ b/tests/regression/75-invalid_dealloc/03-invalid-dealloc-array.c @@ -0,0 +1,25 @@ +#include + +typedef struct custom_t { + int x; + int y; +} custom_t; + +#define MAX_SIZE 5000 + +int main(int argc, char const *argv[]) +{ + custom_t custom_arr[MAX_SIZE]; + free(custom_arr); //WARN + + int int_arr[MAX_SIZE]; + free(int_arr); //WARN + + char char_arr[MAX_SIZE]; + free(char_arr); //WARN + + char char_arr2[1]; + free(char_arr2); //WARN + + return 0; +} diff --git a/tests/regression/75-invalid_dealloc/04-invalid-realloc.c b/tests/regression/75-invalid_dealloc/04-invalid-realloc.c new file mode 100644 index 0000000000..94cbf031c2 --- /dev/null +++ b/tests/regression/75-invalid_dealloc/04-invalid-realloc.c @@ -0,0 +1,25 @@ +#include + +typedef struct custom_t { + int x; + int y; +} custom_t; + +#define MAX_SIZE 5000 + +int main(int argc, char const *argv[]) +{ + custom_t custom_arr[10]; + realloc(custom_arr, MAX_SIZE); //WARN + + int int_arr[100]; + realloc(int_arr, MAX_SIZE); //WARN + + char char_arr[1000]; + realloc(char_arr, MAX_SIZE); //WARN + + char char_arr2[1]; + realloc(char_arr2, MAX_SIZE); //WARN + + return 0; +} From acb1ca83d00ae5f09167e1b30b775a465736ecd1 Mon Sep 17 00:00:00 2001 From: Stanimir Bozhilov Date: Sat, 8 Jul 2023 10:59:19 +0200 Subject: [PATCH 04/10] Warn once for invalid de-/realloc for entire points-to set This includes the case where the points-to-set is top --- src/analyses/base.ml | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/src/analyses/base.ml b/src/analyses/base.ml index cdc8e5a7d3..556aa4440e 100644 --- a/src/analyses/base.ml +++ b/src/analyses/base.ml @@ -1998,18 +1998,14 @@ struct invalidate ~deep:true ~ctx (Analyses.ask_of_ctx ctx) gs st' deep_addrs let check_free_of_non_heap_mem ctx special_fn ptr = - match ctx.ask (Queries.MayPointTo ptr) with - | a when not (Queries.LS.is_top a) -> - let warn_if_not_heap_var special_fn var = - if not (ctx.ask (Queries.IsHeapVar var)) then - M.warn ~category:(Behavior (Undefined InvalidMemoryDeallocation)) ~tags:[CWE 590] "Free of non-dynamically allocated memory in function %s for pointer %a" special_fn.vname d_exp ptr - in - let pointed_to_vars = - Queries.LS.elements a - |> List.map fst - in - List.iter (warn_if_not_heap_var special_fn) pointed_to_vars - | _ -> () + let points_to_set = ctx.ask (Queries.MayPointTo ptr) in + let exists_non_heap_var = + Queries.LS.elements points_to_set + |> List.map fst + |> List.exists (fun var -> not (ctx.ask (Queries.IsHeapVar var))) + in + if exists_non_heap_var then + M.warn ~category:(Behavior (Undefined InvalidMemoryDeallocation)) ~tags:[CWE 590] "Free of non-dynamically allocated memory in function %s for pointer %a" special_fn.vname d_exp ptr let special ctx (lv:lval option) (f: varinfo) (args: exp list) = let invalidate_ret_lv st = match lv with From ee80fd1761a7a780e46c2e9af25389bd268af65e Mon Sep 17 00:00:00 2001 From: Stanimir Bozhilov Date: Sat, 8 Jul 2023 11:30:04 +0200 Subject: [PATCH 05/10] Handle case when points-to set is top --- src/analyses/base.ml | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/analyses/base.ml b/src/analyses/base.ml index 556aa4440e..a4a674cf2d 100644 --- a/src/analyses/base.ml +++ b/src/analyses/base.ml @@ -1999,13 +1999,17 @@ struct let check_free_of_non_heap_mem ctx special_fn ptr = let points_to_set = ctx.ask (Queries.MayPointTo ptr) in - let exists_non_heap_var = - Queries.LS.elements points_to_set - |> List.map fst - |> List.exists (fun var -> not (ctx.ask (Queries.IsHeapVar var))) - in - if exists_non_heap_var then - M.warn ~category:(Behavior (Undefined InvalidMemoryDeallocation)) ~tags:[CWE 590] "Free of non-dynamically allocated memory in function %s for pointer %a" special_fn.vname d_exp ptr + begin try + let exists_non_heap_var = + (* elements throws Unsupported if the points-to set is top *) + Queries.LS.elements points_to_set + |> List.map fst + |> List.exists (fun var -> not (ctx.ask (Queries.IsHeapVar var))) + in + if exists_non_heap_var then + M.warn ~category:(Behavior (Undefined InvalidMemoryDeallocation)) ~tags:[CWE 590] "Free of non-dynamically allocated memory in function %s for pointer %a" special_fn.vname d_exp ptr + with _ -> M.warn ~category:(Behavior (Undefined InvalidMemoryDeallocation)) ~tags:[CWE 590] "Points-to set for pointer %a in function %s is top. Potential free of non-dynamically allocated memory may occur" d_exp ptr special_fn.vname + end let special ctx (lv:lval option) (f: varinfo) (args: exp list) = let invalidate_ret_lv st = match lv with From 5d5afffbe85250e3542c8c7e5adf82ed7427ff9d Mon Sep 17 00:00:00 2001 From: Simmo Saan Date: Wed, 26 Jul 2023 13:46:28 +0300 Subject: [PATCH 06/10] Fix indentation --- src/analyses/base.ml | 8 ++++---- src/cdomains/valueDomain.ml | 20 ++++++++++---------- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/analyses/base.ml b/src/analyses/base.ml index 3a231ea396..0e766401d9 100644 --- a/src/analyses/base.ml +++ b/src/analyses/base.ml @@ -2348,10 +2348,10 @@ struct set ~ctx ~t_override:t ask ctx.global ctx.local (AD.of_var !longjmp_return) t rv (* Not raising Deadcode here, deadcode is raised at a higher level! *) | Rand, _ -> begin match lv with - | Some x -> - let result:value = (Int (ID.starting IInt Z.zero)) in - set ~ctx (Analyses.ask_of_ctx ctx) gs st (eval_lv (Analyses.ask_of_ctx ctx) ctx.global st x) (Cilfacade.typeOfLval x) result - | None -> st + | Some x -> + let result:value = (Int (ID.starting IInt Z.zero)) in + set ~ctx (Analyses.ask_of_ctx ctx) gs st (eval_lv (Analyses.ask_of_ctx ctx) ctx.global st x) (Cilfacade.typeOfLval x) result + | None -> st end | _, _ -> let st = diff --git a/src/cdomains/valueDomain.ml b/src/cdomains/valueDomain.ml index 4e40f3a287..11763ce5c7 100644 --- a/src/cdomains/valueDomain.ml +++ b/src/cdomains/valueDomain.ml @@ -963,16 +963,16 @@ struct Top end | JmpBuf _, _ -> - (* hack for jmp_buf variables *) - begin match value with - | JmpBuf t -> value (* if actually assigning jmpbuf, use value *) - | Blob(Bot, _, _) -> Bot (* TODO: Stopgap for malloced jmp_bufs, there is something fundamentally flawed somewhere *) - | _ -> - if !AnalysisState.global_initialization then - JmpBuf (JmpBufs.Bufs.empty (), false) (* if assigning global init, use empty set instead *) - else - Top - end + (* hack for jmp_buf variables *) + begin match value with + | JmpBuf t -> value (* if actually assigning jmpbuf, use value *) + | Blob(Bot, _, _) -> Bot (* TODO: Stopgap for malloced jmp_bufs, there is something fundamentally flawed somewhere *) + | _ -> + if !AnalysisState.global_initialization then + JmpBuf (JmpBufs.Bufs.empty (), false) (* if assigning global init, use empty set instead *) + else + Top + end | _ -> let result = match offs with From 9f1e5f96eb7f95fbc3e13c693e7874bf4b21db6b Mon Sep 17 00:00:00 2001 From: Simmo Saan Date: Wed, 26 Jul 2023 13:47:57 +0300 Subject: [PATCH 07/10] Fix unused-rec-flag warning in ValueDomain --- src/cdomains/valueDomain.ml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cdomains/valueDomain.ml b/src/cdomains/valueDomain.ml index 11763ce5c7..20c4f3bf21 100644 --- a/src/cdomains/valueDomain.ml +++ b/src/cdomains/valueDomain.ml @@ -564,7 +564,7 @@ struct warn_type "join" x y; Top - let rec widen x y = + let widen x y = match (x,y) with | (Top, _) -> Top | (_, Top) -> Top @@ -582,7 +582,7 @@ struct | (Struct x, Struct y) -> Struct (Structs.widen x y) | (Union x, Union y) -> Union (Unions.widen x y) | (Array x, Array y) -> Array (CArrays.widen x y) - | (Blob x, Blob y) -> Blob (Blobs.widen x y) + | (Blob x, Blob y) -> Blob (Blobs.widen x y) (* TODO: why no blob special cases like in join? *) | (Thread x, Thread y) -> Thread (Threads.widen x y) | (Int x, Thread y) | (Thread y, Int x) -> From 91388efe40f0ea7f432068bfa34c91cce00a82b1 Mon Sep 17 00:00:00 2001 From: Simmo Saan Date: Fri, 28 Jul 2023 12:42:59 +0300 Subject: [PATCH 08/10] Fix UnionDomain crash on chrony --- src/analyses/base.ml | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/analyses/base.ml b/src/analyses/base.ml index 0e766401d9..1808014654 100644 --- a/src/analyses/base.ml +++ b/src/analyses/base.ml @@ -1411,9 +1411,13 @@ struct let new_value = VD.update_offset (Queries.to_value_domain_ask a) old_value offs projected_value lval_raw ((Var x), cil_offset) t in if WeakUpdates.mem x st.weak then VD.join old_value new_value - else if invariant then + else if invariant then ( (* without this, invariant for ambiguous pointer might worsen precision for each individual address to their join *) - VD.meet old_value new_value + try + VD.meet old_value new_value + with Lattice.Uncomparable -> + new_value + ) else new_value in From 99fb013038d3b6c333f236537668e3513d8faceb Mon Sep 17 00:00:00 2001 From: Stanimir Bozhilov Date: Fri, 28 Jul 2023 13:59:17 +0200 Subject: [PATCH 09/10] Refactor and avoid unnecessary queries and conversions --- src/analyses/base.ml | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/src/analyses/base.ml b/src/analyses/base.ml index a4a674cf2d..3a2f4b52bd 100644 --- a/src/analyses/base.ml +++ b/src/analyses/base.ml @@ -1998,18 +1998,14 @@ struct invalidate ~deep:true ~ctx (Analyses.ask_of_ctx ctx) gs st' deep_addrs let check_free_of_non_heap_mem ctx special_fn ptr = - let points_to_set = ctx.ask (Queries.MayPointTo ptr) in - begin try - let exists_non_heap_var = - (* elements throws Unsupported if the points-to set is top *) - Queries.LS.elements points_to_set - |> List.map fst - |> List.exists (fun var -> not (ctx.ask (Queries.IsHeapVar var))) - in - if exists_non_heap_var then - M.warn ~category:(Behavior (Undefined InvalidMemoryDeallocation)) ~tags:[CWE 590] "Free of non-dynamically allocated memory in function %s for pointer %a" special_fn.vname d_exp ptr - with _ -> M.warn ~category:(Behavior (Undefined InvalidMemoryDeallocation)) ~tags:[CWE 590] "Points-to set for pointer %a in function %s is top. Potential free of non-dynamically allocated memory may occur" d_exp ptr special_fn.vname - end + match eval_rv_address (Analyses.ask_of_ctx ctx) ctx.global ctx.local ptr with + | Address a -> + let points_to_set = addrToLvalSet a in + if Q.LS.is_top points_to_set then + M.warn ~category:(Behavior (Undefined InvalidMemoryDeallocation)) ~tags:[CWE 590] "Points-to set for pointer %a in function %s is top. Potential free of non-dynamically allocated memory may occur" d_exp ptr special_fn.vname + else if Q.LS.exists (fun (v, _) -> not (ctx.ask (Q.IsHeapVar v))) points_to_set then + M.warn ~category:(Behavior (Undefined InvalidMemoryDeallocation)) ~tags:[CWE 590] "Free of non-dynamically allocated memory in function %s for pointer %a" special_fn.vname d_exp ptr + | _ -> M.warn ~category:MessageCategory.Analyzer "Pointer %a in function %s doesn't evaluate to a valid address." d_exp ptr special_fn.vname let special ctx (lv:lval option) (f: varinfo) (args: exp list) = let invalidate_ret_lv st = match lv with From e23c42d570ee2539a0f04c10ebd175254eab5065 Mon Sep 17 00:00:00 2001 From: Stanimir Bozhilov Date: Sun, 30 Jul 2023 11:43:18 +0200 Subject: [PATCH 10/10] Add check for UnknownPtr --- src/analyses/base.ml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/analyses/base.ml b/src/analyses/base.ml index 3a2f4b52bd..d4ba662e97 100644 --- a/src/analyses/base.ml +++ b/src/analyses/base.ml @@ -2003,7 +2003,7 @@ struct let points_to_set = addrToLvalSet a in if Q.LS.is_top points_to_set then M.warn ~category:(Behavior (Undefined InvalidMemoryDeallocation)) ~tags:[CWE 590] "Points-to set for pointer %a in function %s is top. Potential free of non-dynamically allocated memory may occur" d_exp ptr special_fn.vname - else if Q.LS.exists (fun (v, _) -> not (ctx.ask (Q.IsHeapVar v))) points_to_set then + else if (Q.LS.exists (fun (v, _) -> not (ctx.ask (Q.IsHeapVar v))) points_to_set) || (AD.mem Addr.UnknownPtr a) then M.warn ~category:(Behavior (Undefined InvalidMemoryDeallocation)) ~tags:[CWE 590] "Free of non-dynamically allocated memory in function %s for pointer %a" special_fn.vname d_exp ptr | _ -> M.warn ~category:MessageCategory.Analyzer "Pointer %a in function %s doesn't evaluate to a valid address." d_exp ptr special_fn.vname