complete.cpp does not parse PATH directories with parenthesis correctly.

[Forethinker]

I am having this problem in Windows, but I have a feeling it will happen in other OS as well.
When I try to autocomplete any command, I get the following message 11 times:

fish: The file 'x86' is not executable by this user
in command substitution
        called on standard input,

This corresponds to the number of directory that includes "Program Files (x86)" in PATH.

I have no trouble running binaries in these directories. For example, firefox is correctly identified as a binary and I can execute it, even though it is in '/cygdrive/c/Program Files (x86)/Mozilla Firefox/firefox.exe'.

[zanchey]

Yes, I can reproduce this:

mkdir ~/\(blah\)
set -gx PATH ~/\(blah\)
ln -s /bin/tar ~/\(blah\)/somenewcommand
somenew<tab>

in command substitution
        called on standard input,

somenew

[xfix]

~ $ tfish: Unknown command 'X86'
in command substitution
        called on standard input,

~ $ echo $PATH
/usr/local/bin /usr/bin /cygdrive/c/windows/SYSTEM32 /cygdrive/c/windows /cygdrive/c/windows/SYSTEM32/WBEM /cygdrive/c/windows/SYSTEM32/WINDOWSPOWERSHELL/V1.0 /cygdrive/c/PROGRAM FILES (X86)/ATI TECHNOLOGIES/ATI.ACE/CORE-STATIC /cygdrive/c/Program Files/Microsoft SQL Server/110/Tools/Binn /bin

X86. Great. Thanks for helping me finding the issue. I think the PATH contents are evaled for some reason. I will try fixing this, I guess. The issue is not only in Cygwin, but I assume it shouldn't happen in sane UNIX environment, considering standard paths on Windows and Linux (there is almost no reason to use symbols in pathes in Linux).

/ $ mkdir '(fail)'
/ $ set PATH '/(fail)' $PATH
/ $ tfish: Unknown command 'fail'
in command substitution
        called on standard input,

fish: Unknown command 'X86'
in command substitution
        called on standard input,

[xfix]

After some experimentation, I found out it also applies to CDPATH expansion (when using cd).

[ridiculousfish]

The issue here is in completer_t::complete_cmd, which invokes expand_string. This is a very general function that expands things like wildcards, process expansion, variable expansion, etc.

It's surely wrong to do these expansions in the components of $PATH. It's somewhat less wrong to do them in the expansions of commands, i.e. if I do this:

/usr/local/bin/pyt?<tab>

What should it expand to? Should it turn into 'python' or 'pyt?on' or nothing at all?

[ridiculousfish]

The current behavior is sort of silly. When executing a command containing a wildcard, it calls expand_one. So if the wildcard has a single match, that is executed; if there are multiple matches, it prints the misleading "Illegal command name" error.

[ridiculousfish]

I feel a little better about fish after comparing it to other shells.

mkdir tests ; cd tests
ln -s /bin/echo boo ; ln -s /bin/echo bar
./b*

zsh, bash, and tcsh all expand the wildcard ./b* into ./bar and ./boo and then execute the command "./bar ./boo", i.e. pass boo as an argument to bar. fish is weird but other shells are insane.

[zanchey]

Perhaps that should replace "a command-line shell for the 90s" as our tagline.

[hsiktas]

I just installed fish from the cygwin repositories and I see this as well.

[xfix]

@HakanSiktas: Strange, I assumed I "fixed" very specific case of (x86) and (X86) by making new functions called x86 and X86 (assuming you installed fish from Cygwin repositories).

But well, the issue not fixed, I just applied a quick hack in order to make this build feel less buggy. Of course it still needs fixing, and it would be nice to get it fixed in the next release. It's actually surprisingly important on one specific platform called Windows, where one of directories is called "Program Files (x86)". I think it could be considered unimportant bug, but I think that Windows makes it more important than it seems initially.

EDIT: Now that I look into it, the patch wasn't applied. Oops. Well, you may want to put this to ~/.config/fish/config.fish. This is temponary workaround, at least until the issue will be fixed.

function x86
    echo '(x86)'
end
function X86
    echo '(X86)'
end

[ridiculousfish]

Hah, that's brilliant!

[Jubijub]

When do you plan to release a patch ? This makes using fish under Cygwin very annoying (which is a shame as fish does seem to be a killer shell :) )

[ridiculousfish]

This is currently slated for the next release, but there's not an ETA yet.

[b-]

After installing fish on msys2, I'm getting this bug. It seems we still haven't quite fixed it... Hoping this bumps it higher up because I really rely on msys2/cygwin for work, and fish makes it so much more comfortable.

[oysteinkrog]

Also seeing this.

[faho]

Is this an issue in complete.cpp or actually in the completion scripts, especially "cd"s (hint: #2289)?

I've just tried

mkdir "Program Files (x86)"
cd Prog<TAB>

(with that PR applied)

and it worked.

[ridiculousfish]

Yay!

[zanchey]

This still isn't fixed:

mkdir -p ~/\(blah\)
ln -s /bin/tar ~/\(blah\)/somenewcommand
set -gx PATH ~/\(blah\) $PATH

Then type somenew and hit Tab.

fish: Unknown command 'blah'
in command substitution
        called on standard input

[jrobeson]

the specific instance of Program Files (x86) would apply to wine users on OSX and linux as well. I've definitely run into it myself.

[krader1961]

If no one else is working on fixing this I'll take a stab at it.

[justinmayer]

That would be great, Kurtis. Much appreciated!

[zanchey]

There are concerns (many thanks to Josh for raising them) that this can be used to cause privilege escalation or arbitrary code execution for other users.

Since the cd completion has been fixed to avoid eval, I haven't managed to reproduce a similar problem without adding a directory with brackets to the path; if you can convince someone to do set PATH /(rm -rf /) then I suspect you can convince them to run arbitrary code regardless.

I thought about trying to add a symlink to a bracket-containing directory to the path, but the symlink is not derefenced so that can't be used.

[zanchey]

In other words, although I think a definitive fix would be great, I don't think this needs to block 2.3.0.

[ridiculousfish]

Thanks zanchey. I'll also look to see how feasible a fix is.

[floam]

if you can convince someone to do set PATH /(rm -rf /)

Or convince someone to do basically anything.

[ridiculousfish]

I have a fix for this in #2912. It's nice because it unifies completions for commands (which need to respect PATH) with those for cd (which respect CDPATH). @krader1961 (or anyone) do you want to review?

[krader1961]

The change proposed by @ridiculousfish fixes the core issue. But, FWIW, other problems remain. For example, my $CDPATH contains the parent directory where I keep all the third-party projects I track. If I cd ~/tmp and type cd ./fish followed by [tab] it still enumerates the two fish directories in my third-party tree. Since that happens with and without the aforementioned change I recommend it be merged. We can tackle the other unexpected behaviors in a separate issue if anyone is bothered by them.

[ridiculousfish]

@krader1961 that problem is due to __fish_complete_cd. We might consider just removing __fish_complete_cd, since the C++ one should be pretty good.

[faho]

@krader1961: Should be fixed in d7c690b.

We might consider just removing __fish_complete_cd, since the C++ one should be pretty good.

Do we already have a C++ completion for enterable directories that takes into account $CDPATH?

[ridiculousfish]

Yes - the logic is there to service the autosuggestion.

[krader1961]

FWIW, @faho's change fixed the cd ./fish[tab] problem I noted. I also removed the share/completions/cd.fish file from my system and didn't notice any ill effects. So, as @ridiculousfish, suggested it might be a good idea to simply remove the __fish_complete_cd function.