diff --git a/infer/annotations/com/facebook/infer/annotation/ThreadSafeMethod.java b/infer/annotations/com/facebook/infer/annotation/ThreadSafeMethod.java new file mode 100644 index 000000000..99e29355e --- /dev/null +++ b/infer/annotations/com/facebook/infer/annotation/ThreadSafeMethod.java @@ -0,0 +1,22 @@ +/* + * Copyright (c) 2017 - present Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + */ + +package com.facebook.infer.annotation; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +/** like javax.annotation.concurrent.ThreadSafe, but for individual methods rather than classes */ + +@Target(ElementType.METHOD) +@Retention(RetentionPolicy.CLASS) +public @interface ThreadSafeMethod { +} diff --git a/infer/src/checkers/ThreadSafety.ml b/infer/src/checkers/ThreadSafety.ml index 7124edb13..36e578c2e 100644 --- a/infer/src/checkers/ThreadSafety.ml +++ b/infer/src/checkers/ThreadSafety.ml @@ -296,17 +296,19 @@ let is_thread_confined_method tenv pname = PatternMatch.check_current_class_attributes Annotations.ia_is_thread_confined tenv pname +let is_annotated f_annot pdesc = + let annotated_signature = Annotations.get_annotated_signature (Procdesc.get_attributes pdesc) in + let ret_annotation, _ = annotated_signature.Annotations.ret in + f_annot ret_annotation (* we don't want to warn on methods that run on the UI thread because they should always be single-threaded *) let runs_on_ui_thread proc_desc = (* assume that methods annotated with @UiThread or @OnEvent(SomeEvent.class) always run on the UI thread *) - let is_annotated pdesc = - let annotated_signature = Annotations.get_annotated_signature (Procdesc.get_attributes pdesc) in - let ret_annotation, _ = annotated_signature.Annotations.ret in - Annotations.ia_is_ui_thread ret_annotation || Annotations.ia_is_on_event ret_annotation in - is_annotated proc_desc + is_annotated + (fun annot -> Annotations.ia_is_ui_thread annot || Annotations.ia_is_on_event annot) + proc_desc (* return true if we should compute a summary for the procedure. if this returns false, we won't analyze the procedure or report any warnings on it *) @@ -421,17 +423,6 @@ let report_thread_safety_violations ( _, tenv, pname, pdesc) trace = report_one_path (PathDomain.get_reportable_sink_paths trace ~trace_of_pname) -(* For now, just checks if there is one active element amongst the posts of the analyzed methods. - This indicates that the method races with itself. To be refined later. *) -let process_results_table tab = - ResultsTableType.iter (* report errors for each method *) - (fun proc_env (_, _, writes) -> - if should_report_on_proc proc_env then - report_thread_safety_violations proc_env writes - else () - ) - tab - (* Currently we analyze if there is an @ThreadSafe annotation on at least one of the classes in a file. This might be tightened in future or even broadened in future based on other criteria *) @@ -450,21 +441,21 @@ let should_report_on_file file_env = not (IList.exists current_class_marked_not_threadsafe file_env) && IList.exists current_class_or_super_marked_threadsafe file_env +(* For now, just checks if there is one active element amongst the posts of the analyzed methods. + This indicates that the method races with itself. To be refined later. *) +let process_results_table file_env tab = + let should_report_on_all_procs = should_report_on_file file_env in + let should_report ((_, _, _, pdesc) as proc_env) = + (should_report_on_all_procs || is_annotated Annotations.ia_is_thread_safe_method pdesc) + && should_report_on_proc proc_env in + ResultsTableType.iter (* report errors for each method *) + (fun proc_env (_, _, writes) -> + if should_report proc_env then report_thread_safety_violations proc_env writes) + tab + (*This is a "cluster checker" *) (*Gathers results by analyzing all the methods in a file, then post-processes the results to check (approximation of) thread safety *) (* file_env: (Idenv.t * Tenv.t * Procname.t * Procdesc.t) list *) let file_analysis _ _ get_procdesc file_env = - let tab = make_results_table get_procdesc file_env in - if should_report_on_file file_env then - process_results_table tab - else () - - (* - Todo: - 0. Refactor abstract domain to use records rather than tuples - 1. Track line numbers of accesses - 2. Track protected writes and reads, too; if we have a write and a read where - one is non-protected then we have potential race (including protected write, unprotected read - 3. Lotsa other stuff - *) + process_results_table file_env (make_results_table get_procdesc file_env) diff --git a/infer/src/checkers/annotations.ml b/infer/src/checkers/annotations.ml index ad099abd0..53f586727 100644 --- a/infer/src/checkers/annotations.ml +++ b/infer/src/checkers/annotations.ml @@ -121,6 +121,7 @@ let integrity_source = "IntegritySource" let integrity_sink = "IntegritySink" let guarded_by = "GuardedBy" let thread_safe = "ThreadSafe" +let thread_safe_method = "ThreadSafeMethod" let not_thread_safe = "NotThreadSafe" let ui_thread = "UiThread" let any_thread = "AnyThread" @@ -134,6 +135,9 @@ let ia_is_not_thread_safe ia = let ia_is_thread_safe ia = ia_ends_with ia thread_safe +let ia_is_thread_safe_method ia = + ia_ends_with ia thread_safe_method + let ia_is_nullable ia = ia_ends_with ia nullable diff --git a/infer/src/checkers/annotations.mli b/infer/src/checkers/annotations.mli index 0002e13a4..692b888bd 100644 --- a/infer/src/checkers/annotations.mli +++ b/infer/src/checkers/annotations.mli @@ -101,6 +101,7 @@ val ia_is_integrity_sink : Annot.Item.t -> bool val ia_is_guarded_by : Annot.Item.t -> bool val ia_is_not_thread_safe : Annot.Item.t -> bool val ia_is_thread_safe : Annot.Item.t -> bool +val ia_is_thread_safe_method : Annot.Item.t -> bool val ia_is_ui_thread : Annot.Item.t -> bool val ia_is_thread_confined : Annot.Item.t -> bool diff --git a/infer/tests/codetoanalyze/java/threadsafety/ThreadSafeExample.java b/infer/tests/codetoanalyze/java/threadsafety/ThreadSafeExample.java index 713686fae..c81993184 100644 --- a/infer/tests/codetoanalyze/java/threadsafety/ThreadSafeExample.java +++ b/infer/tests/codetoanalyze/java/threadsafety/ThreadSafeExample.java @@ -9,6 +9,8 @@ package codetoanalyze.java.checkers; +import com.facebook.infer.annotation.ThreadSafeMethod; + @ThreadSafe public class ThreadSafeExample{ @@ -131,3 +133,14 @@ class YesThreadSafeExtendsNotThreadSafeExample extends NotThreadSafeExtendsThrea } } + +class NonThreadSafeClass { + + Object field; + + @ThreadSafeMethod + public void threadSafeMethod() { + this.field = new Object(); // should warn + } + +} diff --git a/infer/tests/codetoanalyze/java/threadsafety/issues.exp b/infer/tests/codetoanalyze/java/threadsafety/issues.exp index 69fc0bff6..0516dfbcc 100644 --- a/infer/tests/codetoanalyze/java/threadsafety/issues.exp +++ b/infer/tests/codetoanalyze/java/threadsafety/issues.exp @@ -17,6 +17,7 @@ codetoanalyze/java/threadsafety/Ownership.java, void Ownership.ownInOneBranchBad codetoanalyze/java/threadsafety/Ownership.java, void Ownership.reassignToFormalBad(Obj), 2, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.Obj.g] codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ExtendsThreadSafeExample.newmethodBad(), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.ExtendsThreadSafeExample.field] codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ExtendsThreadSafeExample.tsOK(), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.ExtendsThreadSafeExample.field] +codetoanalyze/java/threadsafety/ThreadSafeExample.java, void NonThreadSafeClass.threadSafeMethod(), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.NonThreadSafeClass.field] codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ThreadSafeExample.callPublicMethodBad(), 1, THREAD_SAFETY_VIOLATION, [call to void ThreadSafeExample.assignInPrivateMethodOk(),access to codetoanalyze.java.checkers.ThreadSafeExample.f] codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ThreadSafeExample.deeperTraceBad(), 1, THREAD_SAFETY_VIOLATION, [call to void ThreadSafeExample.callAssignInPrivateMethod(),call to void ThreadSafeExample.assignInPrivateMethodOk(),access to codetoanalyze.java.checkers.ThreadSafeExample.f] codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ThreadSafeExample.oddBad(), 1, THREAD_SAFETY_VIOLATION, [call to void ThreadSafeExample.evenOk(),access to codetoanalyze.java.checkers.ThreadSafeExample.f]