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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 6, 2011, 22:35 UTC
Message-ID: <7vpqg1e3au.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1323207503-26581-2-git-send-email-fransklaver@gmail.com>

Frans Klaver <fransklaver@gmail.com> writes:

Show 13 quoted lines

> +#ifndef WIN32
> +static int is_in_group(gid_t gid)
> ...
> +static int have_read_execute_permissions(const char *path)
> +{
> +	struct stat s;
> +	trace_printf("checking '%s'\n", path);
> +
> +	if (stat(path, &s) < 0) {
> + ...
> +	/* check world permissions */
> +	if ((s.st_mode&(S_IXOTH|S_IROTH)) == (S_IXOTH|S_IROTH))
> +		return 1;

Hmm, do you need to do this with stat(2)?

Wouldn't access(2) with R_OK|X_OK give you exactly what you want without this much trouble?

I also think that your permission check is incorrectly implemented.

    $ cd /var/tmp && date >j && chmod 044 j && ls -l j
    ----r--r-- 1 junio junio 29 Dec  6 14:32 j
    $ cat j
    cat: j: Permission denied
    $ su pogo
    Password:
    $ cat j
    Tue Dec  6 14:32:23 PST 2011
    
That's a world-readable but unreadable-only-to-me file.

> +static void diagnose_execvp_eacces(const char *cmd, const char **argv)
> +{
> +	/* man 2 execve states that EACCES is returned for:

	/*
         * Just a style, but we tend to write multi-line comment like
         * this, without anything else on opening and closing lines of
         * the comment block.
         */

Show 11 quoted lines

> +	 * - The file system is mounted noexec
> +	 */
> +	struct strbuf sb = STRBUF_INIT;
> +	char *path = getenv("PATH");
> +	char *next;
> +
> +	if (strchr(cmd, '/')) {
> +		if (!have_read_execute_permissions(cmd))
> +			error("no read/execute permissions on '%s'\n", cmd);
> +		return;
> +	}

Ok, execvp() failed and "cmd" has at least one slash, so we know we did not look for it in $PATH. We check only one and return (did you need getenv() in that case?).

Show 6 quoted lines

> +	for (;;) {
> +		next = strchrnul(path, ':');
> +		if (path < next)
> +			strbuf_add(&sb, path, next - path);
> +		else
> +			strbuf_addch(&sb, '.');

Nice touch that you did not forget an empty component on $PATH.

> +		if (!have_read_execute_permissions(sb.buf))
> +			error("no read/execute permissions on '%s'\n", sb.buf);

Don't you want to continue here upon error, after resetting sb? You just saw the directory is unreadble, so you know next file_exists() will fail before you try it.

Show 13 quoted lines

> +		if (sb.len && sb.buf[sb.len - 1] != '/')
> +			strbuf_addch(&sb, '/');
> +		strbuf_addstr(&sb, cmd);
> +
> +		if (file_exists(sb.buf)) {
> +			if (!have_read_execute_permissions(sb.buf))
> +				error("no read/execute permissions on '%s'\n",
> +						sb.buf);
> +			else
> +				warn("file '%s' exists and permissions "
> +				"seem OK.\nIf this is a script, see if you "
> +				"have sufficient privileges to run the "
> +				"interpreter", sb.buf);

Does "warn()" do the right thing for multi-line strings like this?