[self in block] Add a check for strongSelf not checked for null

Summary:
The variable strongSelf, because it is equal to a weak captured pointer, needs to be checked for null before being used, otherwise it could be null by the time the block is executed.

Added this check to the SelfInBlock checker, and removed it from biabduction.

We want to migrate all the objc checks from biabduction, so it will be easier to change and faster and more reliable. Moreover, this check is more general, it will flag any use of unchecked strongSelf, not just a dereference.

Reviewed By: skcho

Differential Revision: D18403849

fbshipit-source-id: a9cf5d80b
master
Dulma Churchill 5 years ago committed by Facebook Github Bot
parent 0aec8a04e9
commit bf581e0b72

@ -475,6 +475,7 @@ OPTIONS
STARVATION (enabled by default),
STATIC_INITIALIZATION_ORDER_FIASCO (enabled by default),
STRICT_MODE_VIOLATION (enabled by default),
STRONG_SELF_NOT_CHECKED (enabled by default),
Symexec_memory_error (enabled by default),
TAINTED_BUFFER_ACCESS (enabled by default),
TAINTED_MEMORY_ALLOCATION (enabled by default),

@ -217,6 +217,7 @@ OPTIONS
STARVATION (enabled by default),
STATIC_INITIALIZATION_ORDER_FIASCO (enabled by default),
STRICT_MODE_VIOLATION (enabled by default),
STRONG_SELF_NOT_CHECKED (enabled by default),
Symexec_memory_error (enabled by default),
TAINTED_BUFFER_ACCESS (enabled by default),
TAINTED_MEMORY_ALLOCATION (enabled by default),

@ -475,6 +475,7 @@ OPTIONS
STARVATION (enabled by default),
STATIC_INITIALIZATION_ORDER_FIASCO (enabled by default),
STRICT_MODE_VIOLATION (enabled by default),
STRONG_SELF_NOT_CHECKED (enabled by default),
Symexec_memory_error (enabled by default),
TAINTED_BUFFER_ACCESS (enabled by default),
TAINTED_MEMORY_ALLOCATION (enabled by default),

@ -200,14 +200,6 @@ let deref_str_nullable proc_name_opt nullable_obj_str =
deref_str_null proc_name_opt
(** dereference strings for null dereference due to weak captured variable in block *)
let deref_str_weak_variable_in_block proc_name_opt nullable_obj_str =
let tags = Tags.create () in
Tags.update tags Tags.weak_captured_var_src nullable_obj_str ;
let problem_str = "" in
deref_str_null_ proc_name_opt problem_str
(** dereference strings for nonterminal nil arguments in c/objc variadic methods *)
let deref_str_nil_argument_in_variadic_method pn total_args arg_number =
let function_method, nil_null =

@ -69,9 +69,6 @@ val deref_str_null : Typ.Procname.t option -> deref_str
val deref_str_nullable : Typ.Procname.t option -> string -> deref_str
(** dereference strings for null dereference due to Nullable annotation *)
val deref_str_weak_variable_in_block : Typ.Procname.t option -> string -> deref_str
(** dereference strings for null dereference due to weak captured variable in block *)
val deref_str_undef : Typ.Procname.t * Location.t -> deref_str
(** dereference strings for an undefined value coming from the given procedure *)

@ -345,6 +345,8 @@ let missing_fld = register_from_string "Missing_fld" ~hum:"Missing Field"
let missing_required_prop = register_from_string "MISSING_REQUIRED_PROP"
let mixed_self_weakself = register_from_string "MIXED_SELF_WEAKSELF" ~hum:"Mixed Self WeakSelf"
let mutable_local_variable_in_component_file =
register_from_string "MUTABLE_LOCAL_VARIABLE_IN_COMPONENT_FILE"
@ -413,7 +415,9 @@ let strict_mode_violation =
register_from_string "STRICT_MODE_VIOLATION" ~hum:"Strict Mode Violation"
let mixed_self_weakself = register_from_string "MIXED_SELF_WEAKSELF" ~hum:"Mixed Self WeakSelf"
let strong_self_not_checked =
register_from_string "STRONG_SELF_NOT_CHECKED" ~hum:"Strong Self Not Checked"
let symexec_memory_error =
register_from_string "Symexec_memory_error" ~hum:"Symbolic Execution Memory Error"

@ -230,6 +230,8 @@ val missing_fld : t
val missing_required_prop : t
val mixed_self_weakself : t
val mutable_local_variable_in_component_file : t
val null_dereference : t
@ -286,7 +288,7 @@ val static_initialization_order_fiasco : t
val strict_mode_violation : t
val mixed_self_weakself : t
val strong_self_not_checked : t
val symexec_memory_error : t

@ -1463,29 +1463,11 @@ let rec iter_rearrange pname tenv lexp typ_from_instr prop iter inst :
res
let is_weak_captured_var pdesc var_name =
let pname = Procdesc.get_proc_name pdesc in
match pname with
| Block _ ->
let is_weak_captured (var, typ) =
match typ.Typ.desc with
| Typ.Tptr (_, Pk_objc_weak) ->
String.equal var_name (Mangled.to_string var)
| _ ->
false
in
List.exists ~f:is_weak_captured (Procdesc.get_captured pdesc)
| _ ->
false
let var_has_annotation ?(check_weak_captured_var = false) pdesc is_annotation pvar =
let is_weak_captured_var = is_weak_captured_var pdesc (Pvar.to_string pvar) in
let var_has_annotation pdesc is_annotation pvar =
let ann_sig =
Models.get_modelled_annotated_signature_for_biabduction (Procdesc.get_attributes pdesc)
in
AnnotatedSignature.param_has_annot is_annotation pvar ann_sig
|| (check_weak_captured_var && is_weak_captured_var)
let attr_has_annot is_annotation tenv prop exp =
@ -1521,16 +1503,13 @@ let is_strexp_pt_fld_with_annot tenv obj_str is_annotation typ deref_exp (fld, s
(* This returns true if the exp is pointed to only by fields or parameters with a given
annotation. In that case it also returns a string representation of the annotation
recipient. *)
let is_only_pt_by_fld_or_param_with_annot ?(check_weak_captured_var = false) pdesc tenv prop
deref_exp is_annotation =
let is_only_pt_by_fld_or_param_with_annot pdesc tenv prop deref_exp is_annotation =
let obj_str = ref None in
let is_pt_by_fld_or_param_with_annot hpred =
match hpred with
| Sil.Hpointsto (Exp.Lvar pvar, Sil.Eexp ((Exp.Var _ as exp), _), _)
when Exp.equal exp deref_exp ->
let var_has_annotation =
Pvar.is_seed pvar && var_has_annotation ~check_weak_captured_var pdesc is_annotation pvar
in
let var_has_annotation = Pvar.is_seed pvar && var_has_annotation pdesc is_annotation pvar in
if var_has_annotation then obj_str := Some (Pvar.to_string pvar) ;
let procname_str_opt = attr_has_annot is_annotation tenv prop exp in
if Option.is_some procname_str_opt then obj_str := procname_str_opt ;
@ -1547,8 +1526,7 @@ let is_only_pt_by_fld_or_param_with_annot ?(check_weak_captured_var = false) pde
let is_only_pt_by_fld_or_param_nullable pdesc tenv prop deref_exp =
is_only_pt_by_fld_or_param_with_annot ~check_weak_captured_var:true pdesc tenv prop deref_exp
Annotations.ia_is_nullable
is_only_pt_by_fld_or_param_with_annot pdesc tenv prop deref_exp Annotations.ia_is_nullable
let is_only_pt_by_fld_or_param_nonnull pdesc tenv prop deref_exp =
@ -1599,8 +1577,7 @@ let check_dereference_error tenv pdesc (prop : Prop.normal Prop.t) lexp loc =
let deref_str =
match nullable_var_opt with
| Some str ->
if is_weak_captured_var pdesc str then Localise.deref_str_weak_variable_in_block None str
else Localise.deref_str_nullable None str
Localise.deref_str_nullable None str
| None ->
Localise.deref_str_null None
in

@ -8,27 +8,123 @@ open! IStd
module F = Format
module DomainData = struct
type self_pointer_kind = SELF | WEAK_SELF [@@deriving compare]
type self_pointer_kind = SELF | UNCHECKED_STRONG_SELF | CHECKED_STRONG_SELF | WEAK_SELF
[@@deriving compare]
let is_self kind = match kind with SELF -> true | _ -> false
let is_self kind = match kind with SELF -> true | WEAK_SELF -> false
let is_weak_self kind = match kind with WEAK_SELF -> true | _ -> false
let is_weak_self kind = match kind with SELF -> false | WEAK_SELF -> true
let is_unchecked_strong_self kind = match kind with UNCHECKED_STRONG_SELF -> true | _ -> false
let pp_self_pointer_kind fmt kind =
let s = match kind with SELF -> "SELF" | WEAK_SELF -> "WEAK_SELF" in
let s =
match kind with
| CHECKED_STRONG_SELF ->
"CHECKED_STRONG_SELF"
| SELF ->
"SELF"
| UNCHECKED_STRONG_SELF ->
"UNCHECKED_STRONG_SELF"
| WEAK_SELF ->
"WEAK_SELF"
in
F.fprintf fmt "%s" s
let kind_join kind1 kind2 =
if phys_equal kind1 kind2 then kind1
else
match (kind1, kind2) with
| CHECKED_STRONG_SELF, _ | _, CHECKED_STRONG_SELF ->
CHECKED_STRONG_SELF
| _ ->
Logging.die InternalError
"The only unequal kinds that can be joined in this domain are CHECKED_STRONG_SELF and \
UNCHECKED_STRONG_SELF, but found %a, %a"
pp_self_pointer_kind kind1 pp_self_pointer_kind kind2
let kind_leq kind1 kind2 =
if phys_equal kind1 kind2 then true
else
match (kind1, kind2) with
| CHECKED_STRONG_SELF, UNCHECKED_STRONG_SELF ->
false
| UNCHECKED_STRONG_SELF, CHECKED_STRONG_SELF ->
true
| _ ->
Logging.die InternalError
"The only unequal kinds that can be compared in this domain are CHECKED_STRONG_SELF \
and UNCHECKED_STRONG_SELF, but found %a, %a"
pp_self_pointer_kind kind1 pp_self_pointer_kind kind2
type t = {pvar: Pvar.t; typ: Typ.t [@compare.ignore]; loc: Location.t; kind: self_pointer_kind}
[@@deriving compare]
let pp fmt {pvar; typ; loc; kind} =
F.fprintf fmt "%a:%a, at %a (%a)" (Pvar.pp Pp.text) pvar (Typ.pp Pp.text) typ Location.pp loc
pp_self_pointer_kind kind
let join elem1 elem2 =
assert (Pvar.equal elem1.pvar elem2.pvar) ;
{elem1 with kind= kind_join elem1.kind elem2.kind}
let widen ~prev ~next ~num_iters:_ = join prev next
let leq ~lhs ~rhs =
assert (Pvar.equal lhs.pvar rhs.pvar) ;
kind_leq lhs.kind rhs.kind
end
module PPPVar = struct
type t = Pvar.t [@@deriving compare]
let pp = Pvar.pp Pp.text
end
module CheckedForNull = struct
type t = {checked: bool}
let pp fmt {checked} =
let s = match checked with true -> "Checked" | false -> "NotChecked" in
F.fprintf fmt "%s" s
let join {checked= elem1} {checked= elem2} = {checked= AbstractDomain.BooleanAnd.join elem1 elem2}
let widen ~prev ~next ~num_iters:_ = join prev next
let leq ~lhs:{checked= lhs} ~rhs:{checked= rhs} = AbstractDomain.BooleanAnd.leq ~lhs ~rhs
end
module StrongEqualToWeakCapturedVars = AbstractDomain.Map (PPPVar) (CheckedForNull)
module Vars = AbstractDomain.Map (Ident) (DomainData)
module Domain = struct
type t = {vars: Vars.t; strongVars: StrongEqualToWeakCapturedVars.t}
let pp fmt {vars; strongVars} =
F.fprintf fmt "%a@.%a" Vars.pp vars StrongEqualToWeakCapturedVars.pp strongVars
let join lhs rhs =
{ vars= Vars.join lhs.vars rhs.vars
; strongVars= StrongEqualToWeakCapturedVars.join lhs.strongVars rhs.strongVars }
let widen ~prev ~next ~num_iters:_ = join prev next
let leq ~lhs ~rhs =
Vars.leq ~lhs:lhs.vars ~rhs:rhs.vars
&& StrongEqualToWeakCapturedVars.leq ~lhs:lhs.strongVars ~rhs:rhs.strongVars
end
module TransferFunctions = struct
module Domain = AbstractDomain.FiniteSet (DomainData)
module Domain = Domain
module CFG = ProcCfg.Normal
type extras = unit
@ -52,66 +148,127 @@ module TransferFunctions = struct
attributes.ProcAttributes.captured
let exec_null_check_id (astate : Domain.t) id =
try
let elem = Vars.find id astate.vars in
if StrongEqualToWeakCapturedVars.mem elem.pvar astate.strongVars then
let strongVars =
StrongEqualToWeakCapturedVars.add elem.pvar {checked= true} astate.strongVars
in
let vars =
(* We may have added UNCHECKED_STRONG_SELF in the previous Load instr,
but this occurrence is not a bug, since we are checking right here! *)
Vars.add id {elem with kind= CHECKED_STRONG_SELF} astate.vars
in
{Domain.vars; strongVars}
else astate
with Not_found_s _ | Caml.Not_found -> astate
let exec_instr (astate : Domain.t) {ProcData.summary} _cfg_node (instr : Sil.instr) =
let attributes = Summary.get_attributes summary in
match instr with
| Load {e= Lvar pvar; loc; typ} ->
| Load {id; e= Lvar pvar; loc; typ} ->
let vars =
if is_captured_strong_self attributes pvar then
Domain.add {pvar; typ; loc; kind= SELF} astate
Vars.add id {pvar; typ; loc; kind= SELF} astate.vars
else if is_captured_weak_self attributes pvar then
Domain.add {pvar; typ; loc; kind= WEAK_SELF} astate
else astate
Vars.add id {pvar; typ; loc; kind= WEAK_SELF} astate.vars
else
try
let isChecked = StrongEqualToWeakCapturedVars.find pvar astate.strongVars in
if not isChecked.checked then
Vars.add id {pvar; typ; loc; kind= UNCHECKED_STRONG_SELF} astate.vars
else astate.vars
with Not_found_s _ | Caml.Not_found -> astate.vars
in
{astate with vars}
| Store {e1= Lvar pvar; e2= Var id; typ= pvar_typ} ->
let strongVars =
try
let {DomainData.pvar= binding_for_id} = Vars.find id astate.vars in
if is_captured_weak_self attributes binding_for_id && Typ.is_strong_pointer pvar_typ
then StrongEqualToWeakCapturedVars.add pvar {checked= false} astate.strongVars
else astate.strongVars
with Not_found_s _ | Caml.Not_found -> astate.strongVars
in
{astate with strongVars}
| Prune (Var id, _, _, Sil.Ik_if) ->
exec_null_check_id astate id
(* If (strongSelf != nil) or equivalent else branch *)
| Prune (BinOp (Binop.Ne, Var id, e), _, _, Sil.Ik_if)
(* If (!(strongSelf == nil)) or equivalent else branch *)
| Prune (UnOp (LNot, BinOp (Binop.Eq, Var id, e), _), _, _, Sil.Ik_if) ->
if Exp.is_null_literal e then exec_null_check_id astate id else astate
| _ ->
astate
end
let make_trace_elements domain =
let make_trace_mixed_self_weakself domain =
let trace_elems =
TransferFunctions.Domain.fold
(fun {pvar; loc} trace_elems ->
Vars.fold
(fun _ {pvar; loc; kind} trace_elems ->
match kind with
| SELF | WEAK_SELF ->
let trace_elem_desc = F.asprintf "Using %a" (Pvar.pp Pp.text) pvar in
let trace_elem = Errlog.make_trace_element 0 loc trace_elem_desc [] in
trace_elem :: trace_elems )
trace_elem :: trace_elems
| _ ->
trace_elems )
domain []
in
List.sort trace_elems ~compare:(fun {Errlog.lt_loc= loc1} {Errlog.lt_loc= loc2} ->
Location.compare loc1 loc2 )
let report_issues summary domain =
let report_mix_self_weakself_issues summary domain =
let weakSelf_opt =
TransferFunctions.Domain.filter (fun {kind} -> DomainData.is_weak_self kind) domain
|> TransferFunctions.Domain.choose_opt
in
let self_opt =
TransferFunctions.Domain.filter (fun {kind} -> DomainData.is_self kind) domain
|> TransferFunctions.Domain.choose_opt
Vars.filter (fun _ {kind} -> DomainData.is_weak_self kind) domain |> Vars.choose_opt
in
let self_opt = Vars.filter (fun _ {kind} -> DomainData.is_self kind) domain |> Vars.choose_opt in
match (weakSelf_opt, self_opt) with
| Some {pvar= weakSelf; loc= weakLoc}, Some {pvar= self; loc= selfLoc} ->
| Some (_, {pvar= weakSelf; loc= weakLoc}), Some (_, {pvar= self; loc= selfLoc}) ->
let message =
F.asprintf
"This block uses both %a (%a) and %a (%a). This could lead to retain cycles or \
"This block uses both `%a` (%a) and `%a` (%a). This could lead to retain cycles or \
unexpected behavior."
(Pvar.pp Pp.text) weakSelf Location.pp weakLoc (Pvar.pp Pp.text) self Location.pp selfLoc
in
let ltr = make_trace_elements domain in
let ltr = make_trace_mixed_self_weakself domain in
Reporting.log_error summary ~ltr ~loc:selfLoc IssueType.mixed_self_weakself message
| _ ->
()
let report_unchecked_strongself_issues summary domain =
let unchecked_strongSelf_opt =
Vars.filter (fun _ {kind} -> DomainData.is_unchecked_strong_self kind) domain |> Vars.choose_opt
in
match unchecked_strongSelf_opt with
| Some (_, {pvar; loc}) ->
let message =
F.asprintf
"The variable `%a`, equal to a weak pointer to `self`, is used without a check for null \
at %a. This could cause a crash."
(Pvar.pp Pp.text) pvar Location.pp loc
in
Reporting.log_error summary ~loc IssueType.strong_self_not_checked message
| _ ->
()
module Analyzer = AbstractInterpreter.MakeWTO (TransferFunctions)
let checker {Callbacks.exe_env; summary} =
let initial = TransferFunctions.Domain.empty in
let initial = {Domain.vars= Vars.empty; strongVars= StrongEqualToWeakCapturedVars.empty} in
let procname = Summary.get_proc_name summary in
let tenv = Exe_env.get_tenv exe_env procname in
let proc_data = ProcData.make summary tenv () in
( if Typ.Procname.is_objc_block procname then
match Analyzer.compute_post proc_data ~initial with
| Some domain ->
report_issues summary domain
report_mix_self_weakself_issues summary domain.vars ;
report_unchecked_strongself_issues summary domain.vars
| None ->
() ) ;
summary

@ -45,7 +45,6 @@ codetoanalyze/objc/errors/npe/UpdateDict.m, nullable_NSDictionary_objectForKeyed
codetoanalyze/objc/errors/npe/UpdateDict.m, nullable_NSMapTable_objectForKey, 4, NULL_DEREFERENCE, B5, ERROR, [start of procedure nullable_NSMapTable_objectForKey(),Taking true branch,Condition is true,Taking true branch]
codetoanalyze/objc/errors/npe/UpdateDict.m, update_array_with_null, 5, NULL_DEREFERENCE, B1, ERROR, [start of procedure update_array_with_null()]
codetoanalyze/objc/errors/npe/UpdateDict.m, update_dict_with_key_null, 10, NULL_DEREFERENCE, B1, ERROR, [start of procedure update_dict_with_key_null(),Skipping dictionaryWithObjectsAndKeys:: method has no implementation]
codetoanalyze/objc/errors/npe/WeakCapturedVarsNPE.m, objc_blockWeakCapturedA::strongSelfNoCheck_2, 2, NULL_DEREFERENCE, B1, ERROR, [start of procedure block]
codetoanalyze/objc/errors/npe/nil_in_array_literal.m, Arr::insertNilBad, 3, NULL_DEREFERENCE, B1, ERROR, [start of procedure insertNilBad,start of procedure initA,Taking true branch,return from a call to A::initA]
codetoanalyze/objc/errors/npe/nil_in_array_literal.m, Arr::nilInArrayLiteral0, 4, NULL_DEREFERENCE, B1, ERROR, [start of procedure nilInArrayLiteral0]
codetoanalyze/objc/errors/npe/nil_in_array_literal.m, Arr::nilInArrayLiteral1, 4, NULL_DEREFERENCE, B1, ERROR, [start of procedure nilInArrayLiteral1]

@ -33,4 +33,87 @@
};
}
- (void)strongSelfNoCheckNotWeakSelf_good {
__typeof(self) weakSelf = self;
int (^my_block)(BOOL) = ^(BOOL isTapped) {
__strong __typeof(weakSelf) strongSelf = weakSelf;
return strongSelf->x;
};
}
- (void)strongSelfNoCheck_bad {
__weak __typeof(self) weakSelf = self;
int (^my_block)(BOOL) = ^(BOOL isTapped) {
__strong __typeof(weakSelf) strongSelf = weakSelf;
[strongSelf foo];
return 0;
};
}
// very unlikely pattern, but still complies with invariant:
// any use of strongSelf is bad unless checked for null beforehand
- (void)strongSelfNoCheck2_bad {
__weak __typeof(self) weakSelf = self;
int (^my_block)() = ^() {
__strong __typeof(weakSelf) strongSelf = weakSelf;
__strong __typeof(weakSelf) strongSelf2 = strongSelf;
return 0;
};
}
- (void)strongSelfCheckOnce_bad {
__weak __typeof(self) weakSelf = self;
int (^my_block)(BOOL) = ^(BOOL isTapped) {
__strong __typeof(weakSelf) strongSelf = weakSelf;
if (strongSelf) {
[strongSelf foo];
int x = strongSelf->x;
} else {
[strongSelf foo];
}
[strongSelf foo];
if (strongSelf != nil) {
[strongSelf foo];
int x = strongSelf->x;
}
return 0;
};
}
- (void)strongSelfCheck2_good {
__weak __typeof(self) weakSelf = self;
int (^my_block)(BOOL) = ^(BOOL isTapped) {
__strong __typeof(weakSelf) strongSelf = weakSelf;
if (!strongSelf) {
return 0;
} else {
[strongSelf foo];
}
return 0;
};
}
- (void)strongSelfCheck3_bad {
__weak __typeof(self) weakSelf = self;
int (^my_block)(BOOL) = ^(BOOL isTapped) {
__strong __typeof(weakSelf) strongSelf = weakSelf;
int y = strongSelf->x;
if (strongSelf == nil) {
return 0;
}
return strongSelf->x;
};
}
- (void)strongSelfCheck4_good {
__weak __typeof(self) weakSelf = self;
int (^my_block)() = ^() {
__strong __typeof(weakSelf) strongSelf = weakSelf;
if (!strongSelf) {
} else {
}
return 0;
};
}
@end

@ -1 +1,5 @@
codetoanalyze/objc/self-in-block/StrongSelf.m, objc_blockSelfInBlockTest::mixSelfWeakSelf_bad_1, 4, MIXED_SELF_WEAKSELF, no_bucket, ERROR, [Using &weakSelf,Using &self,Using &self]
codetoanalyze/objc/self-in-block/StrongSelf.m, objc_blockSelfInBlockTest::mixSelfWeakSelf_bad_1, 5, MIXED_SELF_WEAKSELF, no_bucket, ERROR, [Using &weakSelf,Using &self,Using &self]
codetoanalyze/objc/self-in-block/StrongSelf.m, objc_blockSelfInBlockTest::strongSelfCheck3_bad_7, 2, STRONG_SELF_NOT_CHECKED, no_bucket, ERROR, []
codetoanalyze/objc/self-in-block/StrongSelf.m, objc_blockSelfInBlockTest::strongSelfCheckOnce_bad_5, 8, STRONG_SELF_NOT_CHECKED, no_bucket, ERROR, []
codetoanalyze/objc/self-in-block/StrongSelf.m, objc_blockSelfInBlockTest::strongSelfNoCheck2_bad_4, 2, STRONG_SELF_NOT_CHECKED, no_bucket, ERROR, []
codetoanalyze/objc/self-in-block/StrongSelf.m, objc_blockSelfInBlockTest::strongSelfNoCheck_bad_3, 2, STRONG_SELF_NOT_CHECKED, no_bucket, ERROR, []

Loading…
Cancel
Save