From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andrew Cooper <andrew.cooper3@citrix.com>
Subject: Re: [PATCH] xenconsole: merge pty access check into
 when it is opened
Date: Sun, 1 Dec 2013 11:41:59 +0000
Message-ID: <529B2086.1080804@citrix.com>
References: <1385782972-22793-1-git-send-email-mattd@bugfuzz.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1385782972-22793-1-git-send-email-mattd@bugfuzz.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Matthew Daley <mattd@bugfuzz.com>, xen-devel@lists.xen.org
Cc: Ian Jackson <ian.jackson@eu.citrix.com>, Ian Campbell <ian.campbell@citrix.com>, Stefano Stabellini <stefano.stabellini@eu.citrix.com>
List-Id: xen-devel@lists.xenproject.org

On 30/11/2013 03:42, Matthew Daley wrote:
> This stops pty_path from being leaked, and removes the toctou race,
> FWIW.
>
> Not sure why it's a separate check to begin with...
>
> Coverity-ID: 1056047
> Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
> ---
>  tools/console/client/main.c |   10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/tools/console/client/main.c b/tools/console/client/main.c
> index 38c856a..c32d3eb 100644
> --- a/tools/console/client/main.c
> +++ b/tools/console/client/main.c
> @@ -116,12 +116,12 @@ static int get_pty_fd(struct xs_handle *xs, char *path, int seconds)
>  			 * disambiguate: just read the pty path */
>  			pty_path = xs_read(xs, XBT_NULL, path, &len);
>  			if (pty_path != NULL) {
> -				if (access(pty_path, R_OK|W_OK) != 0)
> -					continue;
>  				pty_fd = open(pty_path, O_RDWR | O_NOCTTY);
> -				if (pty_fd == -1) 
> -					err(errno, "Could not open tty `%s'", 
> -					    pty_path);
> +				if (pty_fd == -1) {
> +					if (errno != EACCES)
> +						err(errno, "Could not open tty `%s'",
> +							pty_path);

access() can fail for many more reasons than just EACCES.  I would skip
the errno check entirely and always print the error.

~Andrew

> +				}
>  				free(pty_path);
>  			}
>  		}