From a434618f11dee01c7952aac743c581f0457a02db Mon Sep 17 00:00:00 2001 From: James Wu Date: Wed, 28 Feb 2018 16:55:46 -0800 Subject: [PATCH] Make reading from array with [] an error in most contexts Summary: $y = $x[] is banned. This adds a runtime error for its usage. In my tests, $x[] is allowed only in a few contexts: - In the context of a reference, i.e. &$x[] - As an argument to a function - As an lvalue Reviewed By: oulgen Differential Revision: D7058363 fbshipit-source-id: ab959b2d66ddb682874847ffa1ff7abcc33f3431 --- hphp/hack/src/hhbc/emit_env.ml | 19 ++++++++++++------- hphp/hack/src/hhbc/emit_expression.ml | 33 ++++++++++++++++++++++++++------- hphp/test/hhcodegen_failing_tests_zend | 6 ------ 3 files changed, 38 insertions(+), 20 deletions(-) diff --git a/hphp/hack/src/hhbc/emit_env.ml b/hphp/hack/src/hhbc/emit_env.ml index 3c0bdb8453c..f3586a53a96 100644 --- a/hphp/hack/src/hhbc/emit_env.ml +++ b/hphp/hack/src/hhbc/emit_env.ml @@ -9,12 +9,13 @@ *) type t = { - env_pipe_var : Local.t option; - env_scope : Ast_scope.Scope.t; - env_namespace : Namespace_env.env; - env_needs_local_this: bool; - env_jump_targets : Jump_targets.t; - env_in_try : bool + env_pipe_var : Local.t option; + env_scope : Ast_scope.Scope.t; + env_namespace : Namespace_env.env; + env_needs_local_this : bool; + env_jump_targets : Jump_targets.t; + env_in_try : bool; + env_allows_array_append : bool; } type global_state = @@ -75,6 +76,7 @@ let empty = { env_needs_local_this = false; env_jump_targets = Jump_targets.empty; env_in_try = false; + env_allows_array_append = false; } let get_pipe_var env = env.env_pipe_var @@ -83,6 +85,7 @@ let get_namespace env = env.env_namespace let get_needs_local_this env = env.env_needs_local_this let get_jump_targets env = env.env_jump_targets let is_in_try env = env.env_in_try +let does_env_allow_array_append env = env.env_allows_array_append (* Environment is second parameter so we can chain these e.g. * empty |> with_scope scope |> with_namespace ns @@ -98,7 +101,9 @@ let with_pipe_var v env = let make_class_env ast_class = { env_pipe_var = None; env_scope = [Ast_scope.ScopeItem.Class ast_class]; env_namespace = ast_class.Ast.c_namespace; env_needs_local_this = false; - env_jump_targets = Jump_targets.empty; env_in_try = false; } + env_jump_targets = Jump_targets.empty; env_in_try = false; + env_allows_array_append = false; + } let with_try env = { env with env_in_try = true } let do_in_loop_body break_label continue_label ?iter env s f = diff --git a/hphp/hack/src/hhbc/emit_expression.ml b/hphp/hack/src/hhbc/emit_expression.ml index 03a4b346700..b801244db91 100644 --- a/hphp/hack/src/hhbc/emit_expression.ml +++ b/hphp/hack/src/hhbc/emit_expression.ml @@ -2500,6 +2500,8 @@ and emit_array_get_worker ?(no_final=false) ?mode match base_expr, opt_elem_expr with | (pos, A.Array _), None -> Emit_fatal.raise_fatal_parse pos "Can't use array() as base in write context" + | (pos, _), None when not (Emit_env.does_env_allow_array_append env)-> + Emit_fatal.raise_fatal_runtime pos "Can't use [] for reading" | _ -> let local_temp_kind = get_local_temp_kind inout_param_info env opt_elem_expr in @@ -2877,10 +2879,13 @@ and emit_base_worker ~is_object ~notice ~inout_param_info env mode base_offset | Some i -> FPassBaseGC (i, base_offset) ))) 1 - + (* $a[] can not be used as the base of an array get unless as an lval *) + | A.Array_get(_, None) when not (Emit_env.does_env_allow_array_append env) -> + Emit_fatal.raise_fatal_runtime pos "Can't use [] for reading" (* base is in turn array_get - do a specific handling for inout params if necessary *) | A.Array_get(base_expr, opt_elem_expr) -> + let local_temp_kind = get_local_temp_kind inout_param_info env opt_elem_expr in let elem_expr_instrs, elem_stack_size = @@ -3211,7 +3216,11 @@ and emit_args_and_call env call_pos args uargs = ] end end - else next @@ emit_array_get ~need_ref:false env (Some (i, hint)) + else + next @@ emit_array_get + ~need_ref:false + { env with Emit_env.env_allows_array_append = true } + (Some (i, hint)) QueryOp.CGet base_expr opt_elem_expr | A.Obj_get (e1, e2, nullflavor) -> @@ -3904,6 +3913,14 @@ and emit_lval_op_nonlist env pos op e rhs_instrs rhs_stack_size = ] and emit_lval_op_nonlist_steps env op (pos, expr_) rhs_instrs rhs_stack_size = + let env = + match op with + (* Unbelieveably, $test[] += 5; is legal in PHP, but $test[] = $test[] + 5 is not *) + | LValOp.SetRef + | LValOp.Set + | LValOp.SetOp _ + | LValOp.IncDec _ -> { env with Emit_env.env_allows_array_append = true } + | _ -> env in let handle_dollar e final_op = match e with _, A.Lvar id -> @@ -3920,6 +3937,7 @@ and emit_lval_op_nonlist_steps env op (pos, expr_) rhs_instrs rhs_stack_size = final_op op ] | _ -> + let instrs = emit_expr ~need_ref:false env e in instrs, rhs_instrs, @@ -3965,7 +3983,8 @@ and emit_lval_op_nonlist_steps env op (pos, expr_) rhs_instrs rhs_stack_size = index_instrs, rhs_instrs, final_global_op_instrs - + | A.Array_get (_, None) when not (Emit_env.does_env_allow_array_append env) -> + Emit_fatal.raise_fatal_runtime pos "Can't use [] for reading" | A.Array_get (base_expr, opt_elem_expr) -> let mode = match op with @@ -3978,9 +3997,8 @@ and emit_lval_op_nonlist_steps env op (pos, expr_) rhs_instrs rhs_stack_size = base_expr_instrs_end, base_setup_instrs, base_stack_size = - emit_base - ~notice:Notice ~is_object:false - env mode base_offset None base_expr + emit_base ~notice:Notice ~is_object:false env + mode base_offset None base_expr in let mk = get_elem_member_key env rhs_stack_size opt_elem_expr in let total_stack_size = elem_stack_size + base_stack_size in @@ -4079,7 +4097,8 @@ and from_unop op = | A.Uincr | A.Udecr | A.Upincr | A.Updecr | A.Uref | A.Usilence -> emit_nyi "unop - probably does not need translation" -and emit_expr_as_ref env e = emit_expr ~need_ref:true env e +and emit_expr_as_ref env e = + emit_expr ~need_ref:true { env with Emit_env.env_allows_array_append = true} e and emit_unop ~need_ref env pos op e = let unop_instr = Emit_pos.emit_pos_then pos @@ from_unop op in diff --git a/hphp/test/hhcodegen_failing_tests_zend b/hphp/test/hhcodegen_failing_tests_zend index 7a7891b3d12..3d76b626341 100644 --- a/hphp/test/hhcodegen_failing_tests_zend +++ b/hphp/test/hhcodegen_failing_tests_zend @@ -1,8 +1,5 @@ zend/good/Zend/tests/argument_restriction_004.php zend/good/Zend/tests/bug32296.php -zend/good/Zend/tests/bug41351.php -zend/good/Zend/tests/bug41351_2.php -zend/good/Zend/tests/bug41351_3.php zend/good/Zend/tests/bug43332_1.php zend/good/Zend/tests/bug43343.php zend/good/Zend/tests/bug55086.php @@ -11,9 +8,6 @@ zend/good/Zend/tests/class_name_as_scalar_error_001.php zend/good/Zend/tests/class_name_as_scalar_error_002.php zend/good/Zend/tests/closure_005.php zend/good/Zend/tests/closure_014.php -zend/good/Zend/tests/errmsg_006.php -zend/good/Zend/tests/errmsg_007.php -zend/good/Zend/tests/errmsg_008.php zend/good/Zend/tests/errmsg_012.php zend/good/Zend/tests/errmsg_015.php zend/good/Zend/tests/errmsg_019.php -- 2.11.4.GIT