From: Tomi Ollila <tomi.ollila@iki.fi>
To: Daniel Kahn Gillmor <dkg@fifthhorseman.net>,
 Notmuch Mail <notmuch@notmuchmail.org>
Subject: Re: [PATCH 7/9] add a gpg_path value for notmuch_database_t
In-Reply-To: <87mvtgfws4.fsf@alice.fifthhorseman.net>
References: <1449718786-28000-1-git-send-email-dkg@fifthhorseman.net>
 <1449718786-28000-8-git-send-email-dkg@fifthhorseman.net>
 <m21tas7ifa.fsf@guru.guru-group.fi> <87mvtgfws4.fsf@alice.fifthhorseman.net>
User-Agent: Notmuch/0.21+32~g73439f8 (http://notmuchmail.org) Emacs/24.3.1
 (x86_64-unknown-linux-gnu)
Date: Sun, 13 Dec 2015 01:25:17 +0200
Message-ID: <m2fuz75jxe.fsf@guru.guru-group.fi>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: list

On Sat, Dec 12 2015, Daniel Kahn Gillmor <dkg@fifthhorseman.net> wrote:

> On Fri 2015-12-11 17:02:33 -0500, Tomi Ollila wrote:
>> The above code finds gpg/gpg2 (when called w/ these args) from
>> _CS_PATH (seems to be /bin:/usr/bin by default in linux (tried to
>> look how this set in *BSD -- initially it looks like /usr/local/bin
>> not included but... maybe we let them to complain if this is the case
>> ... :/)
>> ... anyway, the full found path is not set anywhere -- how is it found
>> when used (exec*p() using $PATH? :O)
>
> Hm, according to exec(3):
>
>    Special semantics for execlp() and execvp()
>        The execlp(), execvp(), and execvpe() functions duplicate the
>        actions of the shell in searching for an executable file if the
>        specified filename does not contain a slash (/) character.  The
>        file is sought in the colon-separated list of directory pathnames
>        specified in the PATH environment variable.  If this variable
>        isn't defined, the path list defaults to the current directory
>        followed by the list of directories returned by
>        confstr(_CS_PATH).  (This confstr(3) call typically returns the
>        value "/bin:/usr/bin".)
>
> So this code probably also ought to be searching $PATH as well.  yuck.
> You'd think there would be a commonly-available function for doing this
> specific check without having to actually try to exec() something.

Indeed.

Shells seems to be smart and do pre-check through PATH (but not=20
_CS_PATH...(**)) and then exec just with the found path. I straced
one execlp() usage and it na=C3=AFvely attempted to execve() through the
name appended to path (if getting ENOENT); when execve() succeeds
it does not return...

I originally had a fast thought that it was a security feature not
use PATH (which user can screw up ;/), but some safer path set.
Perhaps that is not necessary...

Perhaps the search should go through PATH if it is defined and
if not defined then use confstr(_CS_PATH). If PATH is empty
then not use confstr (but use current dir !!11!!?++?+?? (*))

One thing I forgot from the review:

the #define try_gpg_path(z) is done inside {}:s, so
before closing } there could be
#undef try_gpg_path (to make the "liveness" of it match the scope)


(*) what is the yuckiness of this:

  $ PATH=3D ls
  zsh: command not found: ls

  $ bash -c 'PATH=3D ls'
  bash: ls: No such file or directory

  $ echo '#!/bin/echo this is ./ls ?' >! ./ls
  $ chmod 755 ./ls

  $ PATH=3D ls
  this is ./ls ? ls -F

  $ PATH=3D command ls
  this is ./ls ? ls

  $ bash -c 'PATH=3D ls'
  this is ./ls ? ls

  $ rm ./ls

(**) unset PATH had the same effect in bash and dash but zsh did not=20
     check ./ls after 'unset PATH'

-- I personally think we could just ignore empty components in PATH
(being it like PATH=3D, PATH=3D:/path/to, PATH=3D/path/to: and finally
PATH=3D/path/to::/path/to) (or then not, just be as compliant (and stupid)
as the PATH handling is).

Tomi

>
>          --dkg