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

Frans Klaver [off-list ref] writes:

+#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.
         */

+	 * - 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?).

+	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.

+		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?