From 4dc9dac9e9e7a03dc905b30fdb3b6277aa5471b0 Mon Sep 17 00:00:00 2001 From: Sam Blackshear Date: Tue, 1 Dec 2015 13:46:41 -0800 Subject: [PATCH] getting started on a Nullable annotation checker for obj-c Summary: public This only supports parameters for now, but should be easy to extend to return values and fields. The work of this diff is all in the translation--the task of finding annotations and doing the actual checking is handled by existing code. Reviewed By: akotulski Differential Revision: D2706791 fb-gh-sync-id: 0d706a8 --- infer/src/backend/localise.ml | 5 +- infer/src/backend/rearrange.ml | 4 +- infer/src/checkers/annotations.mli | 2 + infer/src/clang/cFrontend_utils.ml | 8 ++- infer/src/clang/cFrontend_utils.mli | 4 +- infer/src/clang/cMethod_signature.mli | 6 +- infer/src/clang/cMethod_trans.ml | 43 ++++++++---- .../codetoanalyze/objc/errors/npe/nullable.m | 31 +++++++++ infer/tests/endtoend/objc/NullableTest.java | 66 +++++++++++++++++++ 9 files changed, 147 insertions(+), 22 deletions(-) create mode 100644 infer/tests/codetoanalyze/objc/errors/npe/nullable.m create mode 100644 infer/tests/endtoend/objc/NullableTest.java diff --git a/infer/src/backend/localise.ml b/infer/src/backend/localise.ml index f61be4e2c..f4abd3ea1 100644 --- a/infer/src/backend/localise.ml +++ b/infer/src/backend/localise.ml @@ -437,11 +437,12 @@ let dereference_string deref_str value_str access_opt loc = | Some Initialized_automatically -> ["initialized automatically"] in let problem_desc = + let nullable_text = if !Config.curr_language = Config.Java then "@Nullable" else "__nullable" in let problem_str = match Tags.get !tags Tags.nullable_src with | Some nullable_src -> - if nullable_src = value_str then "is annotated with @Nullable and is dereferenced without a null check" - else "is indirectly marked @Nullable (source: " ^ nullable_src ^ ") and is dereferenced without a null check" + if nullable_src = value_str then "is annotated with " ^ nullable_text ^ " and is dereferenced without a null check" + else "is indirectly marked " ^ nullable_text ^ " (source: " ^ nullable_src ^ ") and is dereferenced without a null check" | None -> deref_str.problem_str in [(problem_str ^ " " ^ at_line tags loc)] in value_desc:: access_desc @ problem_desc, None, !tags diff --git a/infer/src/backend/rearrange.ml b/infer/src/backend/rearrange.ml index 825670305..4f9e49354 100644 --- a/infer/src/backend/rearrange.ml +++ b/infer/src/backend/rearrange.ml @@ -967,8 +967,8 @@ let check_dereference_error pdesc (prop : Prop.normal Prop.t) lexp loc = let is_deref_of_nullable = let is_definitely_non_null exp prop = Prover.check_disequal prop exp Sil.exp_zero in - !Config.report_nullable_inconsistency && !Config.curr_language = Config.Java && - not (is_definitely_non_null root prop) && is_only_pt_by_nullable_fld_or_param root in + !Config.report_nullable_inconsistency && not (is_definitely_non_null root prop) + && is_only_pt_by_nullable_fld_or_param root in let relevant_attributes_getters = [ Prop.get_resource_attribute; Prop.get_undef_attribute; diff --git a/infer/src/checkers/annotations.mli b/infer/src/checkers/annotations.mli index d450fe7fc..a972bd5e4 100644 --- a/infer/src/checkers/annotations.mli +++ b/infer/src/checkers/annotations.mli @@ -52,6 +52,8 @@ val get_annotated_signature : ProcAttributes.t -> annotated_signature val get_field_type_and_annotation : Ident.fieldname -> Sil.typ -> (Sil.typ * Sil.item_annotation) option +val nullable : string + val ia_contains : Sil.item_annotation -> string -> bool val ia_has_annotation_with : Sil.item_annotation -> (Sil.annotation -> bool) -> bool diff --git a/infer/src/clang/cFrontend_utils.ml b/infer/src/clang/cFrontend_utils.ml index 4af4022c2..b7dc82a3a 100644 --- a/infer/src/clang/cFrontend_utils.ml +++ b/infer/src/clang/cFrontend_utils.ml @@ -353,12 +353,18 @@ struct | _ -> assert false (*TODO take the attributes into account too. To be done after we get the attribute's arguments. *) - let is_type_nonnull type_ptr attributes = + let is_type_nonnull type_ptr = let open Clang_ast_t in match get_type type_ptr with | Some AttributedType (_, attr_info) -> attr_info.ati_attr_kind = `Nonnull | _ -> false + let is_type_nullable type_ptr = + let open Clang_ast_t in + match get_type type_ptr with + | Some AttributedType (_, attr_info) -> attr_info.ati_attr_kind = `Nullable + | _ -> false + let string_of_type_ptr type_ptr = match get_desugared_type type_ptr with | Some typ -> (Clang_ast_proj.get_type_tuple typ).Clang_ast_t.ti_raw diff --git a/infer/src/clang/cFrontend_utils.mli b/infer/src/clang/cFrontend_utils.mli index d5632f777..edb4e4935 100644 --- a/infer/src/clang/cFrontend_utils.mli +++ b/infer/src/clang/cFrontend_utils.mli @@ -64,7 +64,9 @@ sig val is_copy : Clang_ast_t.property_attribute option -> bool - val is_type_nonnull : Clang_ast_t.type_ptr -> Clang_ast_t.attribute list -> bool + val is_type_nonnull : Clang_ast_t.type_ptr -> bool + + val is_type_nullable : Clang_ast_t.type_ptr -> bool val get_fresh_pointer : unit -> string diff --git a/infer/src/clang/cMethod_signature.mli b/infer/src/clang/cMethod_signature.mli index a98adc47f..94d6e4c23 100644 --- a/infer/src/clang/cMethod_signature.mli +++ b/infer/src/clang/cMethod_signature.mli @@ -31,9 +31,9 @@ val ms_get_lang : method_signature -> CFrontend_config.lang val ms_get_pointer_to_parent : method_signature -> Clang_ast_t.pointer option -val make_ms : Procname.t -> (string * Clang_ast_t.type_ptr) list -> - Clang_ast_t.type_ptr -> Clang_ast_t.attribute list -> Clang_ast_t.source_range -> bool -> - bool -> CFrontend_config.lang -> Clang_ast_t.pointer option -> method_signature +val make_ms : Procname.t -> (string * Clang_ast_t.type_ptr) list -> Clang_ast_t.type_ptr + -> Clang_ast_t.attribute list -> Clang_ast_t.source_range -> bool -> bool -> CFrontend_config.lang + -> Clang_ast_t.pointer option -> method_signature val replace_name_ms : method_signature -> Procname.t -> method_signature diff --git a/infer/src/clang/cMethod_trans.ml b/infer/src/clang/cMethod_trans.ml index 7c3752934..1e107c797 100644 --- a/infer/src/clang/cMethod_trans.ml +++ b/infer/src/clang/cMethod_trans.ml @@ -86,22 +86,22 @@ let get_return_type function_method_decl_info = | Block_decl_info (_, typ, _) -> CTypes.return_type_of_function_type typ | ObjC_Meth_decl_info (method_decl_info, _) -> method_decl_info.Clang_ast_t.omdi_result_type -let build_method_signature decl_info procname function_method_decl_info is_anonym_block is_generated - parent_pointer = +let build_method_signature decl_info procname function_method_decl_info is_anonym_block + is_generated parent_pointer = let source_range = decl_info.Clang_ast_t.di_source_range in let tp = get_return_type function_method_decl_info in let is_instance_method = is_instance_method function_method_decl_info in let parameters = get_parameters function_method_decl_info in let attributes = decl_info.Clang_ast_t.di_attributes in let lang = get_language function_method_decl_info in - CMethod_signature.make_ms procname parameters tp attributes source_range is_instance_method - is_generated lang parent_pointer + CMethod_signature.make_ms + procname parameters tp attributes source_range is_instance_method is_generated + lang parent_pointer -let get_assume_not_null_calls ms param_decls = - let attributes = CMethod_signature.ms_get_attributes ms in +let get_assume_not_null_calls param_decls = let do_one_param decl = match decl with | Clang_ast_t.ParmVarDecl (decl_info, name, tp, var_decl_info) - when CFrontend_utils.Ast_utils.is_type_nonnull tp attributes -> + when CFrontend_utils.Ast_utils.is_type_nonnull tp -> let assume_call = Ast_expressions.create_assume_not_null_call decl_info name tp in [(`ClangStmt assume_call)] | _ -> [] in @@ -121,7 +121,7 @@ let method_signature_of_decl meth_decl block_data_opt = let function_info = Some (decl_info, fdi) in let procname = General_utils.mk_procname_from_function name function_info tp language in let ms = build_method_signature decl_info procname func_decl false false None in - let extra_instrs = get_assume_not_null_calls ms fdi.Clang_ast_t.fdi_parameters in + let extra_instrs = get_assume_not_null_calls fdi.Clang_ast_t.fdi_parameters in ms, fdi.Clang_ast_t.fdi_body, extra_instrs | CXXMethodDecl (decl_info, name_info, tp, fdi, mdi), _ | CXXConstructorDecl (decl_info, name_info, tp, fdi, mdi), _ -> @@ -131,7 +131,7 @@ let method_signature_of_decl meth_decl block_data_opt = let method_decl = Cpp_Meth_decl_info (fdi, mdi, class_name, tp) in let parent_pointer = decl_info.Clang_ast_t.di_parent_pointer in let ms = build_method_signature decl_info procname method_decl false false parent_pointer in - let non_null_instrs = get_assume_not_null_calls ms fdi.Clang_ast_t.fdi_parameters in + let non_null_instrs = get_assume_not_null_calls fdi.Clang_ast_t.fdi_parameters in let init_list_instrs = get_init_list_instrs mdi in (* it will be empty for methods *) ms, fdi.Clang_ast_t.fdi_body, (init_list_instrs @ non_null_instrs) | ObjCMethodDecl (decl_info, name_info, mdi), _ -> @@ -143,14 +143,14 @@ let method_signature_of_decl meth_decl block_data_opt = let method_decl = ObjC_Meth_decl_info (mdi, class_name) in let is_generated = Ast_utils.is_generated name_info in let parent_pointer = decl_info.Clang_ast_t.di_parent_pointer in - let ms = build_method_signature decl_info procname method_decl false is_generated - parent_pointer in - let extra_instrs = get_assume_not_null_calls ms mdi.omdi_parameters in + let ms = + build_method_signature decl_info procname method_decl false is_generated parent_pointer in + let extra_instrs = get_assume_not_null_calls mdi.omdi_parameters in ms, mdi.omdi_body, extra_instrs | BlockDecl (decl_info, bdi), Some (outer_context, tp, procname, _) -> let func_decl = Block_decl_info (bdi, tp, outer_context) in let ms = build_method_signature decl_info procname func_decl true false None in - let extra_instrs = get_assume_not_null_calls ms bdi.bdi_parameters in + let extra_instrs = get_assume_not_null_calls bdi.bdi_parameters in ms, bdi.bdi_body, extra_instrs | _ -> raise Invalid_declaration @@ -282,12 +282,28 @@ let should_create_procdesc cfg procname defined generated = else false | None -> true +let sil_method_annotation_of_args args : Sil.method_annotation = + let default_visibility = true in + let mk_annot param_name annot_name = + let annot = { Sil.class_name = annot_name; Sil.parameters = [param_name]; } in + annot, default_visibility in + let arg_to_sil_annot acc (arg_name, type_ptr) = + if CFrontend_utils.Ast_utils.is_type_nullable type_ptr then + [mk_annot arg_name Annotations.nullable] :: acc + else acc in + let param_annots = IList.fold_left arg_to_sil_annot [] args in + (* TODO: parse annotations on return value *) + let retval_annot = [] in + retval_annot, param_annots + (** Creates a procedure description. *) let create_local_procdesc cfg tenv ms fbody captured is_objc_inst_method = let defined = not ((IList.length fbody) == 0) in let proc_name = CMethod_signature.ms_get_name ms in let pname = Procname.to_string proc_name in let attributes = sil_func_attributes_of_attributes (CMethod_signature.ms_get_attributes ms) in + let method_annotation = + sil_method_annotation_of_args (CMethod_signature.ms_get_args ms) in let is_generated = CMethod_signature.ms_is_generated ms in let is_cpp_inst_method = CMethod_signature.ms_is_instance ms && CMethod_signature.ms_get_lang ms = CFrontend_config.CPP in @@ -316,6 +332,7 @@ let create_local_procdesc cfg tenv ms fbody captured is_objc_inst_method = is_objc_instance_method = is_objc_inst_method; is_cpp_instance_method = is_cpp_inst_method; loc = loc_start; + method_annotation; ret_type; } in Cfg.Procdesc.create { diff --git a/infer/tests/codetoanalyze/objc/errors/npe/nullable.m b/infer/tests/codetoanalyze/objc/errors/npe/nullable.m new file mode 100644 index 000000000..ffb73b4ab --- /dev/null +++ b/infer/tests/codetoanalyze/objc/errors/npe/nullable.m @@ -0,0 +1,31 @@ +/* +* Copyright (c) 2015 - 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. +*/ + +#import + +@interface A : NSObject { + @public int x; +} + +@end + +int derefNullableParamDirect(A * __nullable param) { + return param->x; +} + +int derefNullableParamIndirect(A * __nullable param) { + A* local = param; + return local->x; +} + +A * derefNullableParamOk(A * __nullable param) { + if (!param) param = [A new]; + param->x = 7; + return param; +} diff --git a/infer/tests/endtoend/objc/NullableTest.java b/infer/tests/endtoend/objc/NullableTest.java new file mode 100644 index 000000000..f7bb151f9 --- /dev/null +++ b/infer/tests/endtoend/objc/NullableTest.java @@ -0,0 +1,66 @@ +/* +* Copyright (c) 2013 - 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 endtoend.objc; + +import static org.hamcrest.MatcherAssert.assertThat; +import static utils.matchers.ResultContainsExactly.containsExactly; + +import com.google.common.collect.ImmutableList; + +import org.junit.BeforeClass; +import org.junit.ClassRule; +import org.junit.Test; + +import java.io.IOException; + +import utils.DebuggableTemporaryFolder; +import utils.InferException; +import utils.InferResults; +import utils.InferRunner; + +public class NullableTest { + + public static final String NPE_FILE = "infer/tests/codetoanalyze/objc/errors/npe/nullable.m"; + + private static ImmutableList inferCmdNPD; + + public static final String NULL_DEREFERENCE = "NULL_DEREFERENCE"; + + @ClassRule + public static DebuggableTemporaryFolder folderNPD = new DebuggableTemporaryFolder(); + + @BeforeClass + public static void runInfer() throws InterruptedException, IOException { + inferCmdNPD = InferRunner.createiOSInferCommandWithMLBuckets( + folderNPD, + NPE_FILE, + "cf", + true); + } + + @Test + public void nullDereferenceTest() throws InterruptedException, IOException, InferException { + InferResults inferResults = InferRunner.runInferC(inferCmdNPD); + String[] procedures = { + "derefNullableParamDirect", + "derefNullableParamIndirect" + }; + assertThat( + "Results should contain null pointer dereference error", + inferResults, + containsExactly( + NULL_DEREFERENCE, + NPE_FILE, + procedures + ) + ); + } + +}