Shell: Do not leak the value of ARGV in nested function calls

Author: https://github.com/alimpfard
Commit: https://github.com/SerenityOS/serenity/commit/1aed61964a2
Pull-request: https://github.com/SerenityOS/serenity/pull/3888
Reviewed-by: https://github.com/awesomekling

Shell/Shell.cpp

@@ -413,18 +413,27 @@ String Shell::local_variable_or(const String& name, const String& replacement)
     return replacement;
 }
 
-void Shell::set_local_variable(const String& name, RefPtr<AST::Value> value)
+void Shell::set_local_variable(const String& name, RefPtr<AST::Value> value, bool only_in_current_frame)
 {
-    if (auto* frame = find_frame_containing_local_variable(name))
-        frame->local_variables.set(name, move(value));
-    else
-        m_local_frames.last().local_variables.set(name, move(value));
+    if (!only_in_current_frame) {
+        if (auto* frame = find_frame_containing_local_variable(name)) {
+            frame->local_variables.set(name, move(value));
+            return;
+        }
+    }
+
+    m_local_frames.last().local_variables.set(name, move(value));
 }
 
-void Shell::unset_local_variable(const String& name)
+void Shell::unset_local_variable(const String& name, bool only_in_current_frame)
 {
-    if (auto* frame = find_frame_containing_local_variable(name))
-        frame->local_variables.remove(name);
+    if (!only_in_current_frame) {
+        if (auto* frame = find_frame_containing_local_variable(name))
+            frame->local_variables.remove(name);
+        return;
+    }
+
+    m_local_frames.last().local_variables.remove(name);
 }
 
 void Shell::define_function(String name, Vector<String> argnames, RefPtr<AST::Node> body)
@@ -473,7 +482,7 @@ bool Shell::invoke_function(const AST::Command& command, int& retval)
 
     auto argv = command.argv;
     argv.take_first();
-    set_local_variable("ARGV", adopt(*new AST::ListValue(move(argv))));
+    set_local_variable("ARGV", adopt(*new AST::ListValue(move(argv))), true);
 
     function.body->run(*this);
 

Shell/Shell.h

@@ -103,8 +103,8 @@ public:
     RefPtr<AST::Value> get_argument(size_t);
     RefPtr<AST::Value> lookup_local_variable(const String&);
     String local_variable_or(const String&, const String&);
-    void set_local_variable(const String&, RefPtr<AST::Value>);
-    void unset_local_variable(const String&);
+    void set_local_variable(const String&, RefPtr<AST::Value>, bool only_in_current_frame = false);
+    void unset_local_variable(const String&, bool only_in_current_frame = false);
 
     void define_function(String name, Vector<String> argnames, RefPtr<AST::Node> body);
     bool has_function(const String&);

Shell/Tests/function.sh

@@ -24,3 +24,13 @@ if fn 2>/dev/null {
 fn() { echo $0 }
 
 test "$(fn)" = fn || echo '$0' in function not equal to its name && exit 1
+
+# Ensure ARGV does not leak from inner frames.
+fn() {
+    fn2 1 2 3
+    echo $*
+}
+
+fn2() { }
+
+test "$(fn foobar)" = "foobar" || echo 'Frames are somehow messed up in nested functions' && exit 1