Skip to content

Commit

Permalink
fix: segfault in positional arg completion
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
iFreilicht committed Oct 30, 2023
1 parent 9f70227 commit 40563f6
Show file tree
Hide file tree
Showing 3 changed files with 29 additions and 2 deletions.
13 changes: 12 additions & 1 deletion src/libutil/args.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
14 changes: 13 additions & 1 deletion src/libutil/args.hh
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 4 additions & 0 deletions tests/functional/completions.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 40563f6

Please sign in to comment.