From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-0.8 required=5.0 tests=DKIM_ADSP_CUSTOM_MED, DKIM_INVALID,DKIM_SIGNED,FREEMAIL_FROM,MAILING_LIST_MULTI, RCVD_IN_DNSWL_NONE autolearn=ham autolearn_force=no version=3.4.2 Received: from primenet.com.au (ns1.primenet.com.au [203.24.36.2]) by inbox.vuxu.org (OpenSMTPD) with ESMTP id 4b30d3d9 for ; Sat, 16 Mar 2019 20:03:23 +0000 (UTC) Received: (qmail 7130 invoked by alias); 16 Mar 2019 20:03:10 -0000 Mailing-List: contact zsh-workers-help@zsh.org; run by ezmlm Precedence: bulk X-No-Archive: yes List-Id: Zsh Workers List List-Post: List-Help: List-Unsubscribe: X-Seq: 44132 Received: (qmail 22211 invoked by uid 1010); 16 Mar 2019 20:03:10 -0000 X-Qmail-Scanner-Diagnostics: from mail-yw1-f68.google.com by f.primenet.com.au (envelope-from , uid 7791) with qmail-scanner-2.11 (clamdscan: 0.100.2/25384. spamassassin: 3.4.2. Clear:RC:0(209.85.161.68):SA:0(-2.0/5.0):. Processed in 2.782322 secs); 16 Mar 2019 20:03:10 -0000 X-Envelope-From: charlechaud@gmail.com X-Qmail-Scanner-Mime-Attachments: | X-Qmail-Scanner-Zip-Files: | Received-SPF: pass (ns1.primenet.com.au: SPF record at _netblocks.google.com designates 209.85.161.68 as permitted sender) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:from:date:message-id:subject:to; bh=360RtRkNtyPHFE0eJLUaE4zBlkc4oT9su8WnBOEtc1o=; b=F9K+Pn8xnKQmP5ZedQdWsRIW63bblux50F06jHVkMskVFKEdu6Jc840qTwHCEKG7ln J9/7pz4wSIY9RVYYAQifQx0RA1Tbl7dP3mdz/zg3aQSVThNsJ2ISZqsojBqUubKbVO77 WLfujwvltHkMoQ01hrwI4R5Z4A6bdTj5W5NvosON0OY16UzXQB50Pv4xUssqK269tG35 pypBI/iGXLgIebeeXD64n8MeyBP4Hpqe4mRX/yZRrlXO9Wu/PMlnlyNpZtQxHA/mlZaY AXhQx9U2fGAulM/yU7NISGRboT9ZV0HZsmOBJjNFo5wJoQweCUdLDnhRZh4Ano5ZnYRq FkBw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:from:date:message-id:subject:to; bh=360RtRkNtyPHFE0eJLUaE4zBlkc4oT9su8WnBOEtc1o=; b=t3mmTMbNPYC83IBF0/lYOQkqeUe3m6olJmIVqphrAO3O6aJPaqbaQ6ryEfb0Kcs1xK 8/uBp7KFFcrCdOHPBA6AG10ZRaqEsV9heQGPWMoOlCwSNcwUu3D/nKmaJ/x1n4fTA9Ep 42F5QFKw52ML0O7ONQsLp2pVsK/KUp4Va0E/GV81w4YS25Ix06wkklJwsx9zM3YECJh7 Efk4qqxVKLE1w2trq0nxc35CRI/qpRvDrhCR11iBgEz2JWBOcCfItkCYIsuDLJEtbiWb syLN2Ot8nreUxqjHomZwGl7d76lEOoDuaOEoykodPri0Wm8MSQSjpPalsdeBd0QSgLe/ LvMw== X-Gm-Message-State: APjAAAWwfTIC3Fph7adgxcsdsa2COcL0dEFKk0nGo0ZmsGov06LpvZfe IV9cj52h1bu6ikE6VZTGMER5Uqs3dZtehEGRd/3RE/CD X-Google-Smtp-Source: APXvYqzkyYGLGAI7r2/FAkww5aFvP+gXtE+yGpFZYjEjWgJOlgVfWSf6hsRZO7SZYTxXRDvX+IbGfkuqVv5XfdELV1o= X-Received: by 2002:a81:a405:: with SMTP id b5mr8225168ywh.385.1552766552898; Sat, 16 Mar 2019 13:02:32 -0700 (PDT) MIME-Version: 1.0 From: Charles Blake Date: Sat, 16 Mar 2019 16:01:57 -0400 Message-ID: Subject: Command hashing/autocd bug & possible fixes To: zsh-workers@zsh.org Content-Type: multipart/alternative; boundary="000000000000ce005005843ba274" --000000000000ce005005843ba274 Content-Type: text/plain; charset="UTF-8" First the bug. It happens on any OS. It just requires there to be a command in $PATH whose name collides with a root dir entry such as "usr", "bin", "dev" or "opt". The below creates a collision (assumes usual /dev dir exists), does a setopt AUTOCD, witnesses that working, then failing, then working again. It should reproduce with a zsh -f (but should not be sensitive to much besides unsetopt hashcmds): PATH=/tmp:$PATH; touch /tmp/dev; chmod +x /tmp/dev setopt AUTOCD /dev pwd whence /dev /dev # errors out with permission denied unhash /dev /dev # works again If you do a 'hash -L | grep /dev' after the whence you can see what is amiss is that /dev is being put in as a hash key mapping to /tmp//dev. If you strace you can see that Zsh ends up trying to execve("/dev") which fails with EPERM, hence the message. unsetopt hashcmds also works around/removes the problem. Note that one might think with setopt PATHDIRS in effect that there would be an ambiguity about what should happen. /dev appended to an element of PATH after all hits a real program, /tmp//dev, with the usual Unix path collapse rules. Real programs are supposed to take priority over AUTOCD. However, the man pages for PATHDIRS say it should NOT work for commands that begin with "/", "./" or "../". So, I think it should either do the AUTOCD or the docs are wrong. I think the "/", "./", "../" is a simple, good rule and AUTOCD should go through. This is not entirely theoretical problem. LLVM comes with a program called "opt" & "/opt" is very common. I don't know how popular AUTOCD is, but I've always loved it. A lot of typical root entries are not implausible names for commands. Finally, getting misleading permission denied on an "attempted cd" to a root entry is..worrisome. Now ideas for a fix. About these I am less confident as there may be a tangled web of concerns. Since it makes little sense to call whence with a term that explicitly blocks path search, one possible fix is inside Src/exec.c:findcmd(): diff --git a/Src/exec.c b/Src/exec.c index 042ba065a..e6b20dcb6 100644 --- a/Src/exec.c +++ b/Src/exec.c @@ -834,7 +834,7 @@ findcmd(char *arg0, int docopy, int default_path) return NULL; } cn = (Cmdnam) cmdnamtab->getnode(cmdnamtab, arg0); - if (!cn && isset(HASHCMDS) && !isrelative(arg0)) + if (!cn && isset(HASHCMDS) && !isrelative(arg0) && *arg0 != '/') cn = hashcmd(arg0, path); if ((int) strlen(arg0) > PATH_MAX) return NULL; That tests out fine for my "whence" case, but besides bin_whence(), findcmd() has 6 other call sites: Src/subst.c:equalsubstr(), Src/Zle/zle_tricky.c:docomplete() Src/Zle/zle_tricky.c:expandcmdpath(), Src/Zle/compctl.c:makecomplistpc(), Src/Zle/compctl.c:makecomplistcmd(), and Src/Zle/compctl.c:addmatch(). It's possible those sites are also buggy OR possible they have reason to put an absolute path into cmdnametab. If they have a legitimate need, findcmd() could grow an allowRooted parameter they could pass. That's a more involved patch, obviously. Another possible fix might be to try to stop hashcmd from entering keys with a leading '/' (or accept an allowRooted parameter). The easiest way to do that.. diff --git a/Src/exec.c b/Src/exec.c index 042ba065a..79ef83c1e 100644 --- a/Src/exec.c +++ b/Src/exec.c @@ -940,6 +940,8 @@ hashcmd(char *arg0, char **pp) char *s, buf[PATH_MAX+1]; char **pq; + if (*arg0 == '/') + return NULL; for (; *pp; pp++) if (**pp == '/') { s = buf; ..also tests out fine for my "whence" example. That sidesteps the HASHDIRS optimization only for the (probably rare?) rooted path. If that's viewed as a real problem, the lower isset(HASHDIRS) code could be duplicated before the early return. There are only a few more call sites to hashcmd besides the 6 indirect calls already mentioned, Src/utils.c:spckword(), Src/Zle/zle_tricky.c:docomplete(), and Src/utils.c:execcmd_exec(). Chances seem good that the original concept of cmdnametab explicitly did not want rooted paths in there, but that somewhere in those 9 other call sites someone cares to allow absolute paths for some reason or another. But I do think that once a rooted path is in there, if it collides with a root directory entry the same name collision logic will break autocd. So, finally, it also seems possible to let rooted paths stay and have a smarter isreallycom(): diff --git a/Src/exec.c b/Src/exec.c index 042ba065a..701e8c2c7 100644 --- a/Src/exec.c +++ b/Src/exec.c @@ -906,6 +906,8 @@ isreallycom(Cmdnam cn) else if (!cn->u.name) return 0; else { + if (*cn->node.nam == '/') + return 0; strcpy(fullnam, *(cn->u.name)); strcat(fullnam, "/"); strcat(fullnam, cn->node.nam); This lets "/dev" be put into cmdnametab, but the smarter isreallycmd() prevents that from interfering with autocd. We could also add an "&& isset(AUTOCD) " to that nam == '/' condition, too. isreallycmd() is only called in once place the trycd clause of execcmd_exec(). This is the most conservative approach (and, yes, yes, whatever we go with should all have some comment explaining itself a bit). Does anyone have any firm opinions on what approach they would like beyond the goes-without-saying "don't break stuff"? Should rooted paths be blocked from cmdnametab outright keeping it "clean"? Or should we just workaround their presence? Both? Some other approach entirely? --000000000000ce005005843ba274--