From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] xenconsole: merge pty access check into when it is opened Date: Sun, 1 Dec 2013 23:29:29 +0000 Message-ID: <529BC659.8010602@citrix.com> References: <1385782972-22793-1-git-send-email-mattd@bugfuzz.com> <529B2086.1080804@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Matthew Daley Cc: Stefano Stabellini , Ian Jackson , Ian Campbell , Xen-devel List-Id: xen-devel@lists.xenproject.org On 01/12/2013 23:14, Matthew Daley wrote: > On Mon, Dec 2, 2013 at 12:41 AM, Andrew Cooper > wrote: >> 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 >>> --- >>> 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. > err() doesn't return though (calls exit()). Given that, is always > calling it still acceptable? > > - Matthew Hmm - that is a point. Even as the patch currently stands, the behaviour of the loop has changed. Before, it would eat any error from access(), and only die in the toctou case where open() subsequently failed. The code is waiting for an individual pty path found from xenstore. I would think that any errors resulting from a bad path being present in xenstore should probably count as fatal. After all, this is a xenconsole client asking xenconsoled which pty to connect to (which now I come to think of it, given an implicit assumption that the client is in the same domain as xenconsoled). ~Andrew