From 0e31b8d8d0b19679414d7621086c9ab408d8318c Mon Sep 17 00:00:00 2001 From: Karoliine Holter Date: Thu, 26 Oct 2023 19:02:07 +0300 Subject: [PATCH 01/11] Add unknown thread ID --- src/analyses/useAfterFree.ml | 2 +- src/cdomains/mHP.ml | 2 +- src/cdomains/threadIdDomain.ml | 73 +++++++++++++++++++++++++++++++++- 3 files changed, 74 insertions(+), 3 deletions(-) diff --git a/src/analyses/useAfterFree.ml b/src/analyses/useAfterFree.ml index ef63ab3e91..96a06a6cc1 100644 --- a/src/analyses/useAfterFree.ml +++ b/src/analyses/useAfterFree.ml @@ -76,7 +76,7 @@ struct end else if HeapVars.mem heap_var (snd ctx.local) then begin if is_double_free then set_mem_safety_flag InvalidFree else set_mem_safety_flag InvalidDeref; - M.warn ~category:(Behavior behavior) ~tags:[CWE cwe_number] "%s might occur in current unique thread %a for heap variable %a" bug_name ThreadIdDomain.FlagConfiguredTID.pretty current CilType.Varinfo.pretty heap_var + M.warn ~category:(Behavior behavior) ~tags:[CWE cwe_number] "%s might occur in current unique thread %a for heap variable %a" bug_name ThreadIdDomain.Thread.pretty current CilType.Varinfo.pretty heap_var end end | `Top -> diff --git a/src/cdomains/mHP.ml b/src/cdomains/mHP.ml index 8037cfa21d..016a72a77e 100644 --- a/src/cdomains/mHP.ml +++ b/src/cdomains/mHP.ml @@ -4,7 +4,7 @@ include Printable.Std let name () = "mhp" -module TID = ThreadIdDomain.FlagConfiguredTID +module TID = ThreadIdDomain.Thread module Pretty = GoblintCil.Pretty type t = { diff --git a/src/cdomains/threadIdDomain.ml b/src/cdomains/threadIdDomain.ml index 7193552048..ff6edf8bda 100644 --- a/src/cdomains/threadIdDomain.ml +++ b/src/cdomains/threadIdDomain.ml @@ -279,6 +279,77 @@ struct let name () = "FlagConfiguredTID: " ^ if history_enabled () then H.name () else P.name () end -module Thread = FlagConfiguredTID +module Thread : Stateful = +struct + include Printable.Std + type t = + | Thread of FlagConfiguredTID.t + | UnknownThread + [@@deriving eq, ord, hash] + + let name () = "Thread id" + let pretty () t = + match t with + | Thread tid -> FlagConfiguredTID.pretty () tid + | UnknownThread -> Pretty.text "Unknown thread id" + + let show t = + match t with + | Thread tid -> FlagConfiguredTID.show tid + | UnknownThread -> "Unknown thread id" + + let printXml f t = + match t with + | Thread tid -> FlagConfiguredTID.printXml f tid + | UnknownThread -> BatPrintf.fprintf f "\n\nUnknown thread id\n\n\n" + + let to_yojson t = + match t with + | Thread tid -> FlagConfiguredTID.to_yojson tid + | UnknownThread -> `String "Unknown thread id" + + let relift t = + match t with + | Thread tid -> Thread (FlagConfiguredTID.relift tid) + | UnknownThread -> UnknownThread + + let lift t = Thread t + + let threadinit v ~multiple = Thread (FlagConfiguredTID.threadinit v ~multiple) + + let is_main t = + match t with + | Thread tid -> FlagConfiguredTID.is_main tid + | UnknownThread -> false + + let is_unique t = + match t with + | Thread tid -> FlagConfiguredTID.is_unique tid + | UnknownThread -> false + + let may_create t1 t2 = + match t1, t2 with + | Thread tid1, Thread tid2 -> FlagConfiguredTID.may_create tid1 tid2 + | _, _ -> true + + let is_must_parent t1 t2 = + match t1, t2 with + | Thread tid1, Thread tid2 -> FlagConfiguredTID.is_must_parent tid1 tid2 + | _, _ -> false + + module D = FlagConfiguredTID.D + + let threadenter (t, d) node i v = + match t with + | Thread tid -> List.map lift (FlagConfiguredTID.threadenter (tid, d) node i v) + | UnknownThread -> assert false + + let threadspawn = FlagConfiguredTID.threadspawn + + let created t d = + match t with + | Thread tid -> Option.map (List.map lift) (FlagConfiguredTID.created tid d) + | UnknownThread -> None +end module ThreadLifted = Lift (Thread) From 192108b69ced96430d69148ad360cff22c4e0bf5 Mon Sep 17 00:00:00 2001 From: Karoliine Holter Date: Thu, 26 Oct 2023 19:34:30 +0300 Subject: [PATCH 02/11] Use set instead of toppedSet for ThreadSet --- src/cdomains/concDomain.ml | 21 ++++++++++++++++++++- src/cdomains/threadIdDomain.ml | 12 +++++++----- 2 files changed, 27 insertions(+), 6 deletions(-) diff --git a/src/cdomains/concDomain.ml b/src/cdomains/concDomain.ml index b16cdf1d9f..5f609a31d8 100644 --- a/src/cdomains/concDomain.ml +++ b/src/cdomains/concDomain.ml @@ -1,6 +1,25 @@ (** Domains for thread sets and their uniqueness. *) -module ThreadSet = SetDomain.ToppedSet (ThreadIdDomain.Thread) (struct let topname = "All Threads" end) +module ThreadSet = +struct + include SetDomain.Make (ThreadIdDomain.Thread) + + let is_top = mem UnknownThread + + let top () = singleton UnknownThread + + let merge uop cop x y = + match is_top x, is_top y with + | true, true -> uop x y + | false, true -> x + | true, false -> y + | false, false -> cop x y + + let meet x y = merge join meet x y + + let narrow x y = merge (fun x y -> widen x (join x y)) narrow x y + +end module MustThreadSet = SetDomain.Reverse(ThreadSet) module CreatedThreadSet = ThreadSet diff --git a/src/cdomains/threadIdDomain.ml b/src/cdomains/threadIdDomain.ml index ff6edf8bda..c0a8f2390f 100644 --- a/src/cdomains/threadIdDomain.ml +++ b/src/cdomains/threadIdDomain.ml @@ -279,13 +279,15 @@ struct let name () = "FlagConfiguredTID: " ^ if history_enabled () then H.name () else P.name () end -module Thread : Stateful = +type thread = + | Thread of FlagConfiguredTID.t + | UnknownThread +[@@deriving eq, ord, hash] + +module Thread : Stateful with type t = thread = struct include Printable.Std - type t = - | Thread of FlagConfiguredTID.t - | UnknownThread - [@@deriving eq, ord, hash] + type t = thread [@@deriving eq, ord, hash] let name () = "Thread id" let pretty () t = From 1221860befda16cbdf5c3ca3bc3e1d6a775dca46 Mon Sep 17 00:00:00 2001 From: Karoliine Holter Date: Fri, 27 Oct 2023 12:03:34 +0300 Subject: [PATCH 03/11] Add test for unknown thread id --- .../51-threadjoins/07-trivial-unknowntid.c | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) create mode 100644 tests/regression/51-threadjoins/07-trivial-unknowntid.c diff --git a/tests/regression/51-threadjoins/07-trivial-unknowntid.c b/tests/regression/51-threadjoins/07-trivial-unknowntid.c new file mode 100644 index 0000000000..2797291ee3 --- /dev/null +++ b/tests/regression/51-threadjoins/07-trivial-unknowntid.c @@ -0,0 +1,34 @@ +//PARAM: --set ana.activated[+] threadJoins +#include + +int g = 10; +int h = 10; +pthread_mutex_t A = PTHREAD_MUTEX_INITIALIZER; + +void *t_fun(void *arg) { + g++; // RACE! + return NULL; +} + +void *t_benign(void *arg) { + h++; // NORACE + pthread_t id2; + pthread_create(&id2, NULL, t_fun, NULL); + foo(&id2); + pthread_join(id2, NULL); + return NULL; +} + +int main(void) { + int t; + + pthread_t id2; + pthread_create(&id2, NULL, t_benign, NULL); + pthread_join(id2, NULL); + // t_benign and t_fun should be in here + + g++; // RACE! + h++; // NORACE + + return 0; +} From 2df78822dc866fbb9bd26dbb6ccba893c280f114 Mon Sep 17 00:00:00 2001 From: Karoliine Holter Date: Fri, 27 Oct 2023 12:04:31 +0300 Subject: [PATCH 04/11] Fix unsoundness on unknown function call with tid as argument --- src/cdomains/valueDomain.ml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cdomains/valueDomain.ml b/src/cdomains/valueDomain.ml index cba4b04c18..d3c8bc6989 100644 --- a/src/cdomains/valueDomain.ml +++ b/src/cdomains/valueDomain.ml @@ -708,7 +708,7 @@ struct let v = invalidate_value ask voidType (CArrays.get ask n (array_idx_top)) in Array (CArrays.set ask n (array_idx_top) v) | t , Blob n -> Blob (Blobs.invalidate_value ask t n) - | _ , Thread _ -> state (* TODO: no top thread ID set! *) + | _ , Thread tid -> Thread (Threads.join (Threads.top ()) tid) | _ , JmpBuf _ -> state (* TODO: no top jmpbuf *) | _, Bot -> Bot (* Leave uninitialized value (from malloc) alone in free to avoid trashing everything. TODO: sound? *) | t , _ -> top_value t From a401a68ee26c9d40ee7f2ec0e0a467c93211240a Mon Sep 17 00:00:00 2001 From: Karoliine Holter Date: Fri, 27 Oct 2023 23:25:49 +0300 Subject: [PATCH 05/11] Replace exception handling with top checks --- src/analyses/apron/relationPriv.apron.ml | 22 +++++++++++----------- src/analyses/basePriv.ml | 22 +++++++++++----------- src/analyses/threadAnalysis.ml | 7 ++++--- src/analyses/threadJoins.ml | 4 ++-- 4 files changed, 28 insertions(+), 27 deletions(-) diff --git a/src/analyses/apron/relationPriv.apron.ml b/src/analyses/apron/relationPriv.apron.ml index b386af162b..3adfa272bb 100644 --- a/src/analyses/apron/relationPriv.apron.ml +++ b/src/analyses/apron/relationPriv.apron.ml @@ -1011,17 +1011,17 @@ struct ) ) else ( - match ConcDomain.ThreadSet.elements tids with - | [tid] -> - let lmust',l' = G.thread (getg (V.thread tid)) in - {st with priv = (w, LMust.union lmust' lmust, L.join l l')} - | _ -> - (* To match the paper more closely, one would have to join in the non-definite case too *) - (* Given how we handle lmust (for initialization), doing this might actually be beneficial given that it grows lmust *) - st - | exception SetDomain.Unsupported _ -> - (* elements throws if the thread set is top *) - st + if ConcDomain.ThreadSet.is_top tids + then st + else + match ConcDomain.ThreadSet.elements tids with + | [tid] -> + let lmust',l' = G.thread (getg (V.thread tid)) in + {st with priv = (w, LMust.union lmust' lmust, L.join l l')} + | _ -> + (* To match the paper more closely, one would have to join in the non-definite case too *) + (* Given how we handle lmust (for initialization), doing this might actually be beneficial given that it grows lmust *) + st ) let thread_return ask getg sideg tid (st: relation_components_t) = diff --git a/src/analyses/basePriv.ml b/src/analyses/basePriv.ml index 0154924a1c..ed6439a847 100644 --- a/src/analyses/basePriv.ml +++ b/src/analyses/basePriv.ml @@ -544,17 +544,17 @@ struct ) ) else ( - match ConcDomain.ThreadSet.elements tids with - | [tid] -> - let lmust',l' = G.thread (getg (V.thread tid)) in - {st with priv = (w, LMust.union lmust' lmust, L.join l l')} - | _ -> - (* To match the paper more closely, one would have to join in the non-definite case too *) - (* Given how we handle lmust (for initialization), doing this might actually be beneficial given that it grows lmust *) - st - | exception SetDomain.Unsupported _ -> - (* elements throws if the thread set is top *) - st + if (ConcDomain.ThreadSet.is_top tids) + then st + else + match ConcDomain.ThreadSet.elements tids with + | [tid] -> + let lmust',l' = G.thread (getg (V.thread tid)) in + {st with priv = (w, LMust.union lmust' lmust, L.join l l')} + | _ -> + (* To match the paper more closely, one would have to join in the non-definite case too *) + (* Given how we handle lmust (for initialization), doing this might actually be beneficial given that it grows lmust *) + st ) let thread_return ask getg sideg tid (st: BaseComponents (D).t) = diff --git a/src/analyses/threadAnalysis.ml b/src/analyses/threadAnalysis.ml index 1e679a4707..acc53d9dee 100644 --- a/src/analyses/threadAnalysis.ml +++ b/src/analyses/threadAnalysis.ml @@ -54,15 +54,16 @@ struct | ThreadJoin { thread = id; ret_var } -> (* TODO: generalize ThreadJoin like ThreadCreate *) (let has_clean_exit tid = not (BatTuple.Tuple3.third (ctx.global tid)) in + let tids = ctx.ask (Queries.EvalThread id) in let join_thread s tid = if has_clean_exit tid && not (is_not_unique ctx tid) then D.remove tid s else s in - match TS.elements (ctx.ask (Queries.EvalThread id)) with - | threads -> List.fold_left join_thread ctx.local threads - | exception SetDomain.Unsupported _ -> ctx.local) + if TS.is_top tids + then ctx.local + else List.fold_left join_thread ctx.local (TS.elements tids)) | _ -> ctx.local let query ctx (type a) (q: a Queries.t): a Queries.result = diff --git a/src/analyses/threadJoins.ml b/src/analyses/threadJoins.ml index f2cd36619f..2977ed9082 100644 --- a/src/analyses/threadJoins.ml +++ b/src/analyses/threadJoins.ml @@ -52,7 +52,7 @@ struct if TIDs.is_top threads then ctx.local else ( - (* elements throws if the thread set is top *) + (* all elements are known *) let threads = TIDs.elements threads in match threads with | [tid] when TID.is_unique tid-> @@ -70,7 +70,7 @@ struct (MustTIDs.bot(), true) (* consider everything joined, MustTIDs is reversed so bot is All threads *) ) else ( - (* elements throws if the thread set is top *) + (* all elements are known *) let threads = TIDs.elements threads in if List.compare_length_with threads 1 > 0 then M.info ~category:Unsound "Ambiguous thread ID assume-joined, assuming all of those threads must-joined."; From 4cb8c97c0d35b69dd6cf18452dff49e7453a2666 Mon Sep 17 00:00:00 2001 From: Karoliine Holter Date: Sun, 29 Oct 2023 20:15:50 +0200 Subject: [PATCH 06/11] Join threads with top when joining with int or address --- src/cdomains/valueDomain.ml | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/cdomains/valueDomain.ml b/src/cdomains/valueDomain.ml index d3c8bc6989..f5e9c45845 100644 --- a/src/cdomains/valueDomain.ml +++ b/src/cdomains/valueDomain.ml @@ -552,11 +552,9 @@ struct | y, Blob (x,s,o) -> Blob (join (x:t) y, s, o) | (Thread x, Thread y) -> Thread (Threads.join x y) | (Int x, Thread y) - | (Thread y, Int x) -> - Thread y (* TODO: ignores int! *) + | (Thread y, Int x) -> Thread (Threads.join y (Threads.top ())) | (Address x, Thread y) - | (Thread y, Address x) -> - Thread y (* TODO: ignores address! *) + | (Thread y, Address x) -> Thread (Threads.join y (Threads.top ())) | (JmpBuf x, JmpBuf y) -> JmpBuf (JmpBufs.join x y) | (Mutex, Mutex) -> Mutex | (MutexAttr x, MutexAttr y) -> MutexAttr (MutexAttr.join x y) From ae7a4061ffa1b120c20e3f641a637c197494cc12 Mon Sep 17 00:00:00 2001 From: Karoliine Holter Date: Sun, 29 Oct 2023 20:20:32 +0200 Subject: [PATCH 07/11] Implement widen for threads with int and address similarly to the Address and Int case --- src/cdomains/valueDomain.ml | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/cdomains/valueDomain.ml b/src/cdomains/valueDomain.ml index f5e9c45845..c8b3ac928e 100644 --- a/src/cdomains/valueDomain.ml +++ b/src/cdomains/valueDomain.ml @@ -583,11 +583,9 @@ struct | (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) -> - Thread y (* TODO: ignores int! *) + | (Thread y, Int x) -> Thread (Threads.widen y (Threads.join y (Threads.top ()))) | (Address x, Thread y) - | (Thread y, Address x) -> - Thread y (* TODO: ignores address! *) + | (Thread y, Address x) -> Thread (Threads.widen y (Threads.join y (Threads.top ()))) | (Mutex, Mutex) -> Mutex | (JmpBuf x, JmpBuf y) -> JmpBuf (JmpBufs.widen x y) | (MutexAttr x, MutexAttr y) -> MutexAttr (MutexAttr.widen x y) From 894e6189dfa5a27dbb0872d5feeae23e35568888 Mon Sep 17 00:00:00 2001 From: Karoliine Holter Date: Sun, 29 Oct 2023 21:20:47 +0200 Subject: [PATCH 08/11] Handle top thread when handling thread joins in base --- src/analyses/base.ml | 1 + 1 file changed, 1 insertion(+) diff --git a/src/analyses/base.ml b/src/analyses/base.ml index 6536a9c496..58ab2dc219 100644 --- a/src/analyses/base.ml +++ b/src/analyses/base.ml @@ -2372,6 +2372,7 @@ struct | Int n when GobOption.exists (BI.equal BI.zero) (ID.to_int n) -> st | Address ret_a -> begin match eval_rv (Analyses.ask_of_ctx ctx) gs st id with + | Thread a when ValueDomain.Threads.is_top a -> invalidate ~ctx (Analyses.ask_of_ctx ctx) gs st [ret_var] | Thread a -> let v = List.fold VD.join (VD.bot ()) (List.map (fun x -> G.thread (ctx.global (V.thread x))) (ValueDomain.Threads.elements a)) in (* TODO: is this type right? *) From 0f1389808ff3a848a9d6e6484df3f381860c7ddc Mon Sep 17 00:00:00 2001 From: Karoliine Holter Date: Wed, 1 Nov 2023 17:14:03 +0200 Subject: [PATCH 09/11] Fix indentation --- src/analyses/apron/relationPriv.apron.ml | 22 +++++++++++----------- src/analyses/basePriv.ml | 22 +++++++++++----------- 2 files changed, 22 insertions(+), 22 deletions(-) diff --git a/src/analyses/apron/relationPriv.apron.ml b/src/analyses/apron/relationPriv.apron.ml index 3adfa272bb..2baf4cdca8 100644 --- a/src/analyses/apron/relationPriv.apron.ml +++ b/src/analyses/apron/relationPriv.apron.ml @@ -1011,17 +1011,17 @@ struct ) ) else ( - if ConcDomain.ThreadSet.is_top tids - then st - else - match ConcDomain.ThreadSet.elements tids with - | [tid] -> - let lmust',l' = G.thread (getg (V.thread tid)) in - {st with priv = (w, LMust.union lmust' lmust, L.join l l')} - | _ -> - (* To match the paper more closely, one would have to join in the non-definite case too *) - (* Given how we handle lmust (for initialization), doing this might actually be beneficial given that it grows lmust *) - st + if ConcDomain.ThreadSet.is_top tids then + st + else + match ConcDomain.ThreadSet.elements tids with + | [tid] -> + let lmust',l' = G.thread (getg (V.thread tid)) in + {st with priv = (w, LMust.union lmust' lmust, L.join l l')} + | _ -> + (* To match the paper more closely, one would have to join in the non-definite case too *) + (* Given how we handle lmust (for initialization), doing this might actually be beneficial given that it grows lmust *) + st ) let thread_return ask getg sideg tid (st: relation_components_t) = diff --git a/src/analyses/basePriv.ml b/src/analyses/basePriv.ml index ed6439a847..013a48a2d6 100644 --- a/src/analyses/basePriv.ml +++ b/src/analyses/basePriv.ml @@ -544,17 +544,17 @@ struct ) ) else ( - if (ConcDomain.ThreadSet.is_top tids) - then st - else - match ConcDomain.ThreadSet.elements tids with - | [tid] -> - let lmust',l' = G.thread (getg (V.thread tid)) in - {st with priv = (w, LMust.union lmust' lmust, L.join l l')} - | _ -> - (* To match the paper more closely, one would have to join in the non-definite case too *) - (* Given how we handle lmust (for initialization), doing this might actually be beneficial given that it grows lmust *) - st + if ConcDomain.ThreadSet.is_top tids then + st + else + match ConcDomain.ThreadSet.elements tids with + | [tid] -> + let lmust',l' = G.thread (getg (V.thread tid)) in + {st with priv = (w, LMust.union lmust' lmust, L.join l l')} + | _ -> + (* To match the paper more closely, one would have to join in the non-definite case too *) + (* Given how we handle lmust (for initialization), doing this might actually be beneficial given that it grows lmust *) + st ) let thread_return ask getg sideg tid (st: BaseComponents (D).t) = From e19f87e8c0647ebc84db6de4494d07c4817ab4c1 Mon Sep 17 00:00:00 2001 From: Karoliine Holter Date: Tue, 21 Nov 2023 10:16:58 +0200 Subject: [PATCH 10/11] Add multiple as argument to threadenter in threadIdDomain --- src/cdomains/threadIdDomain.ml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cdomains/threadIdDomain.ml b/src/cdomains/threadIdDomain.ml index a22b692921..d0c3f7b61b 100644 --- a/src/cdomains/threadIdDomain.ml +++ b/src/cdomains/threadIdDomain.ml @@ -342,9 +342,9 @@ struct module D = FlagConfiguredTID.D - let threadenter (t, d) node i v = + let threadenter ~multiple (t, d) node i v = match t with - | Thread tid -> List.map lift (FlagConfiguredTID.threadenter (tid, d) node i v) + | Thread tid -> List.map lift (FlagConfiguredTID.threadenter ~multiple (tid, d) node i v) | UnknownThread -> assert false let threadspawn = FlagConfiguredTID.threadspawn From 54bcf607850d2e8c4dc21310abbba0a32c8959d5 Mon Sep 17 00:00:00 2001 From: Simmo Saan Date: Fri, 8 Dec 2023 10:52:39 +0200 Subject: [PATCH 11/11] Add TODO about shallow ThreadJoin invalidate --- src/analyses/base.ml | 1 + 1 file changed, 1 insertion(+) diff --git a/src/analyses/base.ml b/src/analyses/base.ml index 2e0002dd55..078799bea6 100644 --- a/src/analyses/base.ml +++ b/src/analyses/base.ml @@ -2397,6 +2397,7 @@ struct (* handling thread joins... sort of *) | ThreadJoin { thread = id; ret_var }, _ -> let st' = + (* TODO: should invalidate shallowly? https://github.com/goblint/analyzer/pull/1224#discussion_r1405826773 *) match (eval_rv (Analyses.ask_of_ctx ctx) gs st ret_var) with | Int n when GobOption.exists (BI.equal BI.zero) (ID.to_int n) -> st | Address ret_a ->