Re: [PATCH 1/2] run-command: Add checks after execvp fails with EACCES

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 13, 2011, 19:01 UTC
Message-ID: <7vliqguwhq.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1323788917-4141-2-git-send-email-fransklaver@gmail.com>

Frans Klaver <fransklaver@gmail.com> writes:

Show 21 quoted lines

> +static void diagnose_execvp_eacces(const char *cmd, const char **argv)
> +{
> +	/*
> +	 * man 2 execve states that EACCES is returned for:
> +	 * - Search permission is denied on a component of the path prefix
> +	 *   of cmd or the name of a script interpreter
> +	 * - The file or script interpreter is not a regular file
> +	 * - Execute permission is denied for the file, script or ELF
> +	 *   interpreter
> +	 * - The file system is mounted noexec
> +	 */
> +	struct strbuf sb = STRBUF_INIT;
> +	char *path;
> +	char *next;
> +
> +	if (strchr(cmd, '/')) {
> +		if (!have_read_execute_permissions(cmd))
> +			error("no read/execute permissions on '%s'\n", cmd);
> +		return;
> +	}
> +

Three points.

 - error() gives you a LF at the end, so you do not have to have your own.

 - That "have_..._ions()" is too long and ugly.

 - The only thing you care about this callsite is if you have enough
   permission to execute the "cmd".

In fact, you should not unconditionally require read permissions here.

    $ chmod a-r $(type --path git) && /bin/ls -l $(type --path git)
    --wx--x--x 109 junio junio 5126580 Dec 13 09:47 /home/junio/git-active/bin/git
    $ /home/junio/git-active/bin/git --version
    git version 1.7.8.249.gb1b73

You may need read permission when the file is a script (i.e. not binary executable).

Show 14 quoted lines

> +	path = getenv("PATH");
> +	while (path) {
> +		next = strchrnul(path, ':');
> +		if (path < next)
> +			strbuf_add(&sb, path, next - path);
> +		else
> +			strbuf_addch(&sb, '.');
> +
> +		if (!*next)
> +			path = NULL;
> +		else
> +			path = next + 1;
> +
> +		if (!have_read_execute_permissions(sb.buf)) {

When checking if you can run "foo/bar/baz", directories "foo/" and "foo/bar/" do not have to be readable. They only have to have executable bit to allow descending into them, and typically this is called "searchable" (see man chmod).

    $ mkdir -p /var/tmp/a/b && cp $(type --path git) /var/tmp/a/b/git
    $ chmod 111 /var/tmp/a /var/tmp/a/b
    $ /var/tmp/a/b/git --version
    git version 1.7.8.249.gb1b73

I'd suggest having two helper functions, instead of the single one with overlong "have...ions" name.

 - can_search_directory() checks with access(X_OK);

 - can_execute_file() checks with access(X_OK|R_OK), even though R_OK is
   not always needed.

Use the former here where you check the directory that contains the command, and use the latter up above where you check the command that is supposed to be executable, and also down below after you checked sb.buf is a path to a file that may be the command that is supposed to be executable.

Then patch 2/2 can extend can_execute() to enhance its support for scripts by reading the hash-bang line and validating it, etc.