Assume files with all exec bits set are executable for completions#10050
Assume files with all exec bits set are executable for completions#10050faho wants to merge 4 commits into
Conversation
This checks the st_mode, which we need for other reasons. If it has the user, group *and* other exec bits set, i.e. a mode of 777 or 755 or similar, we assume it is executable and skip the waccess. This can have false-positives in some cases: 1. Capabilities 2. noexec filesystems 3. MAC like SELinux? Of these, capabilities also aren't reflected in access. On a debian system: ``` sudo capsh --drop=CAP_NET_RAW --shell=/usr/bin/fish -- -c 'test -x /usr/bin/ping && echo is executable; ping 8.8.8.8' ``` prints > is executable > exec: Failed to execute process '/usr/bin/ping': No permission. Either suid/sgid is forbidden or you lack capabilities. and strace confirms: > faccessat(AT_FDCWD, "/usr/bin/ping", X_OK) = 0 Bash fails similarly, except the error is a more generic "Operation not permitted" This leaves noexec filesystems and MAC. Since this is for completions, it's probably okay to possibly be wrong in those cases - if it is, the user can just try to execute and will get an error. Especially noexec filesystems shouldn't really be in completions because they'd have to be in $PATH. I have been unable to test MAC, and from a cursory googling those would mostly fail the `stat` already. This speeds up the common case by removing *most* access calls here - of the 2869 binaries in my $PATH, *1* (one) does not have a mode of 777, 555 or 755. It has 750. That means completing an empty command will have 2868 fewer access() calls. This regains all of the speed loss of fish-shell#9699.
There was a problem hiding this comment.
This is a big optimization, and the use of all three exec bits is very clever - that covers the vast majority of commands for me as well, and it eliminates a ton of code relative to #9699 (commit 0b55f08). Agreed that false positives are preferred here, and I verified no false positives on my system for tab-completing an empty command.
One source of false positives not on the list (please add it) is ACLs. Definitely real on macOS, I think real on BSD, and I think on Linux too though I wasn't able to get this to work in testing. On macOS the command:
chmod +a "everyone deny execute" /path/to/command
adds a deny execute ACL, which access respects, resulting in a false positive after this commit. Unfortunately the presence of ACLs is not detectable via stat() - they all require additional syscalls. On the plus side, ACLs are pretty unusual on macOS - none of the commands in my $PATH have them. NFS also supports ACLs which probably introduce the same problem.
I'm OK landing this because it is so simple and because it errors on the side of false positives.
If you want to try something more ambitious - it looks like the lstat() call isn't actually used for much? We could just outright replace it with access in this case - that would keep the number of syscalls the same or even slightly reduce it.
(There's also a possibility of replacing the lstat call with lstatx_np - mentioned only for completeness since lstatx_np is not documented.)
Anyways if you decide to land this, please add ACLs to your nice list, and relnote this as a minor behavior change.
The (l)stat call is used for two things:
The former is pretty but not very useful and we could remove it. What we could do is use the information from the entry_t in the caller - we already did "is_dir", so we either know it's a directory or not via d_type, or we have already done a full stat() and can pass it on. That removes a stat(), so we could do access() instead. We could also get the "link" description back if we read it from the entry_t. If we don't have d_type, we would have to do an lstat if we want that description. |
|
Okay, this is a bit of a mess. 7d02977 removed descriptions for file completions, and that's still active. That means the descriptions here, via file_get_desc, are only triggered for command descriptions. Not ordinary file completions or cd (apparently because that boils down to file completions with directories_only turned on). So we only really get "command"/"command link"/"directory"/"directory link". We're not interested in the specifics of link loops or broken links - any non-usable file can just be thrown away for commands. We're also not interested in the size (which was only enabled for non-regular files because of the terrible "is_executable" naming). That means we only need lstat if we don't have d_type (broken links would be discovered via ordinary stat()). That's if we are interested in the "command link" distinction. If we have d_type, we only do stat for links (and could detect broken links). Because we save a stat for each file, we could do an access() in turn, unless we are interested in keeping this heuristic. I'm going to merge this (with the requested cleanup), and then I'm going to check exactly how much is needed here and make something more complicated - this probably involves throwing out half of file_get_desc, and passing the entry_t down so we can keep the filetype around. |
5277752 to
ee0df19
Compare
|
Alright we're going with #10052, which removes the stat instead, keeping the access(), which doesn't have the inaccuracies. |
This checks the st_mode, which we need for other reasons. If it has the user, group and other exec bits set, i.e. a mode of 777 or 755 or similar, we assume it is executable and skip the waccess.
This can have false-positives (i.e. things offered as completion when they aren't executable) in some cases:
Of these, capabilities also aren't reflected in access. On a debian system:
prints
and strace confirms:
Bash fails similarly, except the error is a more generic "Operation not permitted"
This leaves noexec filesystems and MAC.
Since this is for completions, it's probably okay to possibly be wrong in those cases, and better to have false-positives than the false-negatives of #9699 - if it is a false-positive, the user can just try to execute and will get an error. A false-negative will be invisible
Especially noexec filesystems shouldn't really be in completions because they'd have to be in $PATH.
I have been unable to test MAC, and from a cursory googling those would mostly fail the
statalready.This speeds up the common case by removing most access calls here - of the 2869 binaries in my $PATH, 1 (one) does not have a mode of 777, 555 or 755. It has 750.
Another system has 2170 files in $PATH, none of which doesn't have 777 or 755.
That means completing an empty command will have 2868/2170 fewer access() calls.
This regains all of the speed loss of #9699.
TODOs: