From bedad383b51145d14e6bf1dc5d34ed199ab8c6f9 Mon Sep 17 00:00:00 2001
From: AnotherTest <ali.mpfard@gmail.com>
Date: Sun, 1 Nov 2020 13:28:11 +0330
Subject: [PATCH] Shell: Store LocalFrames as NonnullOwnPtr<LocalFrame> instead

As Vector<T> will relocate objects to resize, we cannot assume that the
address of a specific LocalFrame will stay constant, or that the
reference will not be dangling to begin with.
Fixes an assertion that fires when a frame push causes the Vector to
reallocate.
---
 Shell/AST.cpp   |  4 ++--
 Shell/Shell.cpp | 20 ++++++++++++++------
 Shell/Shell.h   | 16 ++++++++++++----
 3 files changed, 28 insertions(+), 12 deletions(-)

diff --git a/Shell/AST.cpp b/Shell/AST.cpp
index e225eb916e..251a7f05b8 100644
--- a/Shell/AST.cpp
+++ b/Shell/AST.cpp
@@ -1033,7 +1033,7 @@ RefPtr<Value> ForLoop::run(RefPtr<Shell> shell)
         RefPtr<Value> block_value;
 
         {
-            auto frame = shell->push_frame();
+            auto frame = shell->push_frame(String::formatted("for ({})", this));
             shell->set_local_variable(m_variable_name, value);
 
             block_value = m_block->run(shell);
@@ -1585,7 +1585,7 @@ RefPtr<Value> MatchExpr::run(RefPtr<Shell> shell)
         return pattern;
     };
 
-    auto frame = shell->push_frame();
+    auto frame = shell->push_frame(String::formatted("match ({})", this));
     if (!m_expr_name.is_empty())
         shell->set_local_variable(m_expr_name, value);
 
diff --git a/Shell/Shell.cpp b/Shell/Shell.cpp
index 3eaeccec4b..aff307a27d 100644
--- a/Shell/Shell.cpp
+++ b/Shell/Shell.cpp
@@ -463,7 +463,7 @@ bool Shell::invoke_function(const AST::Command& command, int& retval)
         return true;
     }
 
-    auto frame = push_frame();
+    auto frame = push_frame(String::formatted("function {}", function.name));
     size_t index = 0;
     for (auto& arg : function.arguments) {
         ++index;
@@ -489,9 +489,12 @@ String Shell::format(const StringView& source, ssize_t& cursor) const
     return result;
 }
 
-Shell::Frame Shell::push_frame()
+Shell::Frame Shell::push_frame(String name)
 {
-    m_local_frames.empend();
+    m_local_frames.append(make<LocalFrame>(name, decltype(LocalFrame::local_variables) {}));
+#ifdef SH_DEBUG
+    dbg() << "New frame '" << name << "' at " << &m_local_frames.last();
+#endif
     return { m_local_frames, m_local_frames.last() };
 }
 
@@ -505,7 +508,12 @@ Shell::Frame::~Frame()
 {
     if (!should_destroy_frame)
         return;
-    ASSERT(&frames.last() == &frame);
+    if (&frames.last() != &frame) {
+        dbg() << "Frame destruction order violation near " << &frame << " (container = " << this << ") '" << frame.name << "'; current frames:";
+        for (auto& frame : frames)
+            dbg() << "- " << &frame << ": " << frame.name;
+        ASSERT_NOT_REACHED();
+    }
     frames.take_last();
 }
 
@@ -1504,7 +1512,7 @@ void Shell::notify_child_event()
 Shell::Shell()
     : m_default_constructed(true)
 {
-    push_frame().leak_frame();
+    push_frame("main").leak_frame();
 
     int rc = gethostname(hostname, Shell::HostNameSize);
     if (rc < 0)
@@ -1544,7 +1552,7 @@ Shell::Shell(Line::Editor& editor)
     tcsetpgrp(0, getpgrp());
     m_pid = getpid();
 
-    push_frame().leak_frame();
+    push_frame("main").leak_frame();
 
     int rc = gethostname(hostname, Shell::HostNameSize);
     if (rc < 0)
diff --git a/Shell/Shell.h b/Shell/Shell.h
index 65d49cff91..0ead032635 100644
--- a/Shell/Shell.h
+++ b/Shell/Shell.h
@@ -30,6 +30,7 @@
 #include "Parser.h"
 #include <AK/CircularQueue.h>
 #include <AK/HashMap.h>
+#include <AK/NonnullOwnPtrVector.h>
 #include <AK/String.h>
 #include <AK/StringBuilder.h>
 #include <AK/Types.h>
@@ -114,11 +115,18 @@ public:
     RefPtr<Line::Editor> editor() const { return m_editor; }
 
     struct LocalFrame {
+        LocalFrame(const String& name, HashMap<String, RefPtr<AST::Value>> variables)
+            : name(name)
+            , local_variables(move(variables))
+        {
+        }
+
+        String name;
         HashMap<String, RefPtr<AST::Value>> local_variables;
     };
 
     struct Frame {
-        Frame(Vector<LocalFrame>& frames, const LocalFrame& frame)
+        Frame(NonnullOwnPtrVector<LocalFrame>& frames, const LocalFrame& frame)
             : frames(frames)
             , frame(frame)
         {
@@ -128,12 +136,12 @@ public:
         void leak_frame() { should_destroy_frame = false; }
 
     private:
-        Vector<LocalFrame>& frames;
+        NonnullOwnPtrVector<LocalFrame>& frames;
         const LocalFrame& frame;
         bool should_destroy_frame { true };
     };
 
-    [[nodiscard]] Frame push_frame();
+    [[nodiscard]] Frame push_frame(String name);
     void pop_frame();
 
     static String escape_token(const String& token);
@@ -247,7 +255,7 @@ private:
     };
 
     HashMap<String, ShellFunction> m_functions;
-    Vector<LocalFrame> m_local_frames;
+    NonnullOwnPtrVector<LocalFrame> m_local_frames;
     NonnullRefPtrVector<AST::Redirection> m_global_redirections;
 
     HashMap<String, String> m_aliases;