Matthew Daley
2013-Nov-30 03:42 UTC
[PATCH] xenconsole: merge pty access check into when it is opened
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); + } free(pty_path); } } -- 1.7.10.4
Andrew Cooper
2013-Dec-01 11:41 UTC
Re: [PATCH] xenconsole: merge pty access check into when it is opened
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); > } > }
Matthew Daley
2013-Dec-01 23:14 UTC
Re: [PATCH] xenconsole: merge pty access check into when it is opened
On Mon, Dec 2, 2013 at 12:41 AM, Andrew Cooper <andrew.cooper3@citrix.com> 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 <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.err() doesn''t return though (calls exit()). Given that, is always calling it still acceptable? - Matthew
Andrew Cooper
2013-Dec-01 23:29 UTC
Re: [PATCH] xenconsole: merge pty access check into when it is opened
On 01/12/2013 23:14, Matthew Daley wrote:> On Mon, Dec 2, 2013 at 12:41 AM, Andrew Cooper > <andrew.cooper3@citrix.com> 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 <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. > err() doesn''t return though (calls exit()). Given that, is always > calling it still acceptable? > > - MatthewHmm - 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
Matthew Daley
2013-Dec-02 02:41 UTC
[PATCH v2] xenconsole: merge pty access check into when it is opened
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> --- v2: err() if open() fails with EACCES as well tools/console/client/main.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/tools/console/client/main.c b/tools/console/client/main.c index 38c856a..3242008 100644 --- a/tools/console/client/main.c +++ b/tools/console/client/main.c @@ -116,12 +116,9 @@ 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) + err(errno, "Could not open tty `%s''", pty_path); free(pty_path); } } -- 1.7.10.4
Andrew Cooper
2013-Dec-02 10:30 UTC
Re: [PATCH v2] xenconsole: merge pty access check into when it is opened
On 02/12/13 02:41, 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>Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>> --- > v2: err() if open() fails with EACCES as well > > tools/console/client/main.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/tools/console/client/main.c b/tools/console/client/main.c > index 38c856a..3242008 100644 > --- a/tools/console/client/main.c > +++ b/tools/console/client/main.c > @@ -116,12 +116,9 @@ 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) > + err(errno, "Could not open tty `%s''", pty_path); > free(pty_path); > } > }
Ian Jackson
2013-Dec-02 11:51 UTC
Re: [PATCH] xenconsole: merge pty access check into when it is opened
Andrew Cooper writes ("Re: [Xen-devel] [PATCH] xenconsole: merge pty access check into when it is opened"):> On 01/12/2013 23:14, Matthew Daley wrote: > > On Mon, Dec 2, 2013 at 12:41 AM, Andrew Cooper > > <andrew.cooper3@citrix.com> wrote: > >> 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?...> 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.I had the impression there is sometimes a race during domain startup, between a toolstack invoking xenconsole and xenconsoled allocating the pty. Ian.
Andrew Cooper
2013-Dec-02 11:53 UTC
Re: [PATCH] xenconsole: merge pty access check into when it is opened
On 02/12/13 11:51, Ian Jackson wrote:> Andrew Cooper writes ("Re: [Xen-devel] [PATCH] xenconsole: merge pty access check into when it is opened"): >> On 01/12/2013 23:14, Matthew Daley wrote: >>> On Mon, Dec 2, 2013 at 12:41 AM, Andrew Cooper >>> <andrew.cooper3@citrix.com> wrote: >>>> 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? > ... >> 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. > I had the impression there is sometimes a race during domain startup, > between a toolstack invoking xenconsole and xenconsoled allocating the > pty. > > Ian.In which case the xenstore path should be NULL and we wander around loop and wait on the watch again. ~Andrew
Matthew Daley
2013-Dec-13 05:59 UTC
Re: [PATCH v2] xenconsole: merge pty access check into when it is opened
Pingy ping? On Mon, Dec 2, 2013 at 3:41 PM, Matthew Daley <mattd@bugfuzz.com> 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> > --- > v2: err() if open() fails with EACCES as well > > tools/console/client/main.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/tools/console/client/main.c b/tools/console/client/main.c > index 38c856a..3242008 100644 > --- a/tools/console/client/main.c > +++ b/tools/console/client/main.c > @@ -116,12 +116,9 @@ 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) > + err(errno, "Could not open tty `%s''", pty_path); > free(pty_path); > } > } > -- > 1.7.10.4 >
Ian Jackson
2013-Dec-13 17:01 UTC
Re: [PATCH] xenconsole: merge pty access check into when it is opened [and 1 more messages]
Andrew Cooper writes ("Re: [Xen-devel] [PATCH] xenconsole: merge pty access check into when it is opened"):> 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.I concur, and v2 of the patch does the same. This is probably correct as Andrew and I discussed. But the deliberate (or, at least, probably-desirable) behaviour change is not reflected in the commit message which only discusses removing the race and saving a leak. Matthew Daley writes ("Re: [PATCH v2] xenconsole: merge pty access check into when it is opened"):> Pingy ping?Sorry for not making this clear sooner, but: I think this patch is a bugfix and should go into 4.4 so I am happy to apply it but the commit message should be improved. Would you like to do that ? Thanks, Ian.
Matthew Daley
2013-Dec-13 22:54 UTC
Re: [PATCH] xenconsole: merge pty access check into when it is opened [and 1 more messages]
On Sat, Dec 14, 2013 at 6:01 AM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:> Andrew Cooper writes ("Re: [Xen-devel] [PATCH] xenconsole: merge pty access check into when it is opened"): >> 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. > > I concur, and v2 of the patch does the same. This is probably correct > as Andrew and I discussed. > > But the deliberate (or, at least, probably-desirable) behaviour change > is not reflected in the commit message which only discusses removing > the race and saving a leak. > > Matthew Daley writes ("Re: [PATCH v2] xenconsole: merge pty access check into when it is opened"): >> Pingy ping? > > Sorry for not making this clear sooner, but: > > I think this patch is a bugfix and should go into 4.4 so I am happy to > apply it but the commit message should be improved. Would you like to > do that ?Sure thing. v3 on its way... - Matthew> > Thanks, > Ian.
Matthew Daley
2013-Dec-14 01:04 UTC
[PATCH v3] xenconsole: adjust pty opening error checking and handling
Currently we check the pty path received from xenstore with access(); if it indicates that the pty is not accessible, we loop around and wait for a new path to appear in xenstore. This has several issues: * If a path has been written to xenstore, it can be assumed that that pty should already be accessible to xenconsole, and hence any error that occurs while trying to open it should be fatal and not ignored * If access() indicates no access to the pty, the memory allocated for the path is leaked when going around the loop again * The accessibility of the pty could change between the access() and open() calls, leading to a TOCTOU race (this is what Coverity is complaining about). By removing the explicit access() check and just erroring out whenever open() fails, we fix all these issues. Coverity-ID: 1056047 Signed-off-by: Matthew Daley <mattd@bugfuzz.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> --- v3: Improve commit message to explain what behaviours are fixed (and hence changed) tools/console/client/main.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/tools/console/client/main.c b/tools/console/client/main.c index 38c856a..3242008 100644 --- a/tools/console/client/main.c +++ b/tools/console/client/main.c @@ -116,12 +116,9 @@ 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) + err(errno, "Could not open tty `%s''", pty_path); free(pty_path); } } -- 1.7.10.4