From 421845b0cd109d3532beb9dfc06e44169b846505 Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Thu, 7 Oct 2021 11:36:44 +0200 Subject: [PATCH] LibJS: Taint variable environment chain after non-strict direct eval() Since non-strict direct eval() can insert new bindings into a surrounding var scope, we cannot safely cache some assumptions about environment chain layout after eval() has taken place. Since eval() is rare, let's do what other engines do and simply deoptimize in its presence. This patch adds a new "permanently screwed" flag to JS::Environment that will be set on the entire variable environment chain upon non-strict direct eval(). --- Userland/Libraries/LibJS/Runtime/AbstractOperations.cpp | 7 +++++++ .../Libraries/LibJS/Runtime/DeclarativeEnvironment.cpp | 2 +- Userland/Libraries/LibJS/Runtime/Environment.cpp | 9 +++++++++ Userland/Libraries/LibJS/Runtime/Environment.h | 7 +++++++ 4 files changed, 24 insertions(+), 1 deletion(-) diff --git a/Userland/Libraries/LibJS/Runtime/AbstractOperations.cpp b/Userland/Libraries/LibJS/Runtime/AbstractOperations.cpp index 6df25398f31..5630d28f510 100644 --- a/Userland/Libraries/LibJS/Runtime/AbstractOperations.cpp +++ b/Userland/Libraries/LibJS/Runtime/AbstractOperations.cpp @@ -444,6 +444,13 @@ ThrowCompletionOr perform_eval(Value x, GlobalObject& caller_realm, Calle if (strict_eval) variable_environment = lexical_environment; + if (direct == EvalMode::Direct && !strict_eval) { + // NOTE: Non-strict direct eval() forces us to deoptimize variable accesses. + // Mark the variable environment chain as screwed since we will not be able + // to rely on cached environment coordinates from this point on. + variable_environment->set_permanently_screwed_by_eval(); + } + // 18. If runningContext is not already suspended, suspend runningContext. // FIXME: We don't have this concept yet. diff --git a/Userland/Libraries/LibJS/Runtime/DeclarativeEnvironment.cpp b/Userland/Libraries/LibJS/Runtime/DeclarativeEnvironment.cpp index 35141437571..5c4d83d8e42 100644 --- a/Userland/Libraries/LibJS/Runtime/DeclarativeEnvironment.cpp +++ b/Userland/Libraries/LibJS/Runtime/DeclarativeEnvironment.cpp @@ -40,7 +40,7 @@ bool DeclarativeEnvironment::has_binding(FlyString const& name, Optional auto it = m_names.find(name); if (it == m_names.end()) return false; - if (out_index) + if (!is_permanently_screwed_by_eval() && out_index) *out_index = it->value; return true; } diff --git a/Userland/Libraries/LibJS/Runtime/Environment.cpp b/Userland/Libraries/LibJS/Runtime/Environment.cpp index bfebf547183..fa650d9f26e 100644 --- a/Userland/Libraries/LibJS/Runtime/Environment.cpp +++ b/Userland/Libraries/LibJS/Runtime/Environment.cpp @@ -28,4 +28,13 @@ void Environment::visit_edges(Visitor& visitor) visitor.visit(m_outer_environment); } +void Environment::set_permanently_screwed_by_eval() +{ + if (m_permanently_screwed_by_eval) + return; + m_permanently_screwed_by_eval = true; + if (outer_environment()) + outer_environment()->set_permanently_screwed_by_eval(); +} + } diff --git a/Userland/Libraries/LibJS/Runtime/Environment.h b/Userland/Libraries/LibJS/Runtime/Environment.h index 69d27bf31e6..e4e9ad0e52b 100644 --- a/Userland/Libraries/LibJS/Runtime/Environment.h +++ b/Userland/Libraries/LibJS/Runtime/Environment.h @@ -53,6 +53,11 @@ public: virtual char const* class_name() const override { return "Environment"; } + // This flag is set on the entire variable environment chain when direct eval() is performed. + // It is used to disable non-local variable access caching. + bool is_permanently_screwed_by_eval() const { return m_permanently_screwed_by_eval; } + void set_permanently_screwed_by_eval(); + protected: explicit Environment(Environment* parent); @@ -63,6 +68,8 @@ private: GlobalObject* m_global_object { nullptr }; Environment* m_outer_environment { nullptr }; + + bool m_permanently_screwed_by_eval { false }; }; }