netdev - Re: [PATCH iproute2 v2] ipbatch: fix use of 'ip netns exec'

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1373326605.1860.12.camel@bwh-desktop.uk.level5networks.com>
Date:	Tue, 9 Jul 2013 00:36:45 +0100
From:	Ben Hutchings <bhutchings@...arflare.com>
To:	Nicolas Dichtel <nicolas.dichtel@...nd.com>
CC:	<shemminger@...tta.com>, <netdev@...r.kernel.org>,
	<junwei.zhang@...nd.com>
Subject: Re: [PATCH iproute2 v2] ipbatch: fix use of 'ip netns exec'

On Mon, 2013-07-08 at 11:44 +0200, Nicolas Dichtel wrote:
[...]
> --- a/ip/ipnetns.c
> +++ b/ip/ipnetns.c
> @@ -138,6 +138,7 @@ static int netns_exec(int argc, char **argv)
>  	const char *name, *cmd;
>  	char net_path[MAXPATHLEN];
>  	int netns;
> +	int pid, status;
>  
>  	if (argc < 1) {
>  		fprintf(stderr, "No netns name specified\n");
> @@ -185,10 +186,23 @@ static int netns_exec(int argc, char **argv)
>  	/* Setup bind mounts for config files in /etc */
>  	bind_etc(name);
>  
> -	if (execvp(cmd, argv + 1)  < 0)
> -		fprintf(stderr, "exec of \"%s\" failed: %s\n",
> -			cmd, strerror(errno));
> -	return EXIT_FAILURE;
> +	pid = fork();
> +	if (pid < 0)
> +		return EXIT_FAILURE;

On failure, this should emit an error message:
		perror("fork");
and then return -1, surely?

> +	else if (pid > 0)
> +		waitpid(pid, &status, 0);
> +	else {
> +		/* Child */
> +		if (execvp(cmd, argv + 1)  < 0)
> +			fprintf(stderr, "exec of \"%s\" failed: %s\n",
> +				cmd, strerror(errno));
> +		return EXIT_FAILURE;

No, the child must not return from this function.  Otherwise there will
be two processes trying to do the same cleanup on the resources that
they still share.  The child should call _exit(127) if exec*() fails.

> +	}
> +	/* ip must returns the status of the child, but do_cmd() will add a
> +	 * minus to this returned value, so let's add another one here to
> +	 * cancel it.
> +	 */
> +	return -WEXITSTATUS(status);
>  }
>  
>  static int is_pid(const char *str)

WEXITSTATUS() is valid only if waitpid() was successful and WIFEXITED()
yields true.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html