From f282ef5a56f6314d25044af62b4de38c66b29d38 Mon Sep 17 00:00:00 2001
From: Felix Uhl <felix.uhl@outlook.com>
Date: Tue, 24 Oct 2023 22:22:05 +0200
Subject: [PATCH] fix: segfault in positional arg completion

Adding the inputPath as a positional feature uncovered this bug.
As positional argument forms were discarded from the `expectedArgs`
list, their closures were not. When the `.completer` closure was then
called, part of the surrounding object did not exist anymore.

This didn't cause an issue before, but with the new call to
`getEvalState()` in the "inputs" completer in nix/flake.cc, a segfault
was triggered reproducibly on invalid memory access to the `this`
pointer, which was always 0.

The solution of splicing the argument forms into a new list to extend
their lifetime is a bit of a hack, but I was unable to get the "nicer"
iterator-based solution to work.
---
 src/libutil/args.cc             | 13 ++++++++++++-
 src/libutil/args.hh             | 14 +++++++++++++-
 tests/functional/completions.sh |  4 ++++
 3 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/src/libutil/args.cc b/src/libutil/args.cc
index 6bc3cae07..811353c18 100644
--- a/src/libutil/args.cc
+++ b/src/libutil/args.cc
@@ -255,7 +255,18 @@ bool Args::processArgs(const Strings & args, bool finish)
         }
         if (!anyCompleted)
             exp.handler.fun(ss);
-        expectedArgs.pop_front();
+
+        /* Move the list element to the processedArgs. This is almost the same as 
+           `processedArgs.push_back(expectedArgs.front()); expectedArgs.pop_front()`,
+           except that it will only adjust the next and prev pointers of the list
+           elements, meaning the actual contents don't move in memory. This is
+           critical to prevent invalidating internal pointers! */
+        processedArgs.splice(
+            processedArgs.end(),
+            expectedArgs,
+            expectedArgs.begin(),
+            ++expectedArgs.begin());
+
         res = true;
     }
 
diff --git a/src/libutil/args.hh b/src/libutil/args.hh
index ff2bf3cab..e3b41313f 100644
--- a/src/libutil/args.hh
+++ b/src/libutil/args.hh
@@ -200,13 +200,25 @@ protected:
     /**
      * Queue of expected positional argument forms.
      *
-     * Positional arugment descriptions are inserted on the back.
+     * Positional argument descriptions are inserted on the back.
      *
      * As positional arguments are passed, these are popped from the
      * front, until there are hopefully none left as all args that were
      * expected in fact were passed.
      */
     std::list<ExpectedArg> expectedArgs;
+    /**
+     * List of processed positional argument forms.
+     * 
+     * All items removed from `expectedArgs` are added here. After all
+     * arguments were processed, this list should be exactly the same as
+     * `expectedArgs` was before.
+     * 
+     * This list is used to extend the lifetime of the argument forms.
+     * If this is not done, some closures that reference the command
+     * itself will segfault.
+    */
+   std::list<ExpectedArg> processedArgs;
 
     /**
      * Process some positional arugments
diff --git a/tests/functional/completions.sh b/tests/functional/completions.sh
index b9886623a..d3d5bbd48 100644
--- a/tests/functional/completions.sh
+++ b/tests/functional/completions.sh
@@ -44,6 +44,10 @@ EOF
 # Input override completion
 [[ "$(NIX_GET_COMPLETIONS=4 nix build ./foo --override-input '')" == $'normal\na\t' ]]
 [[ "$(NIX_GET_COMPLETIONS=5 nix flake show ./foo --override-input '')" == $'normal\na\t' ]]
+cd ./foo
+[[ "$(NIX_GET_COMPLETIONS=3 nix flake update '')" == $'normal\na\t' ]]
+cd ..
+[[ "$(NIX_GET_COMPLETIONS=5 nix flake update --flake './foo' '')" == $'normal\na\t' ]]
 ## With multiple input flakes
 [[ "$(NIX_GET_COMPLETIONS=5 nix build ./foo ./bar --override-input '')" == $'normal\na\t\nb\t' ]]
 ## With tilde expansion