thr3ads.net - Xen devel - [PATCH] xenconsole: merge pty access check into when it is opened [Nov 2013]

If this information is useful, please help other people find it:
Share via:

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

head link

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

head link

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

head link

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

Matthew Daley

2013-Dec-02 02:41 UTC

head link

[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

head link

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

head link

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

head link

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

head link

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

head link

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

head link

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

head link

[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

Xen devel - Nov 2013 - [PATCH] xenconsole: merge pty access check into when it is opened

[PATCH] xenconsole: merge pty access check into when it is opened

Re: [PATCH] xenconsole: merge pty access check into when it is opened

Re: [PATCH] xenconsole: merge pty access check into when it is opened

Re: [PATCH] xenconsole: merge pty access check into when it is opened

[PATCH v2] xenconsole: merge pty access check into when it is opened

Re: [PATCH v2] xenconsole: merge pty access check into when it is opened

Re: [PATCH] xenconsole: merge pty access check into when it is opened

Re: [PATCH] xenconsole: merge pty access check into when it is opened

Re: [PATCH v2] xenconsole: merge pty access check into when it is opened

Re: [PATCH] xenconsole: merge pty access check into when it is opened [and 1 more messages]

Re: [PATCH] xenconsole: merge pty access check into when it is opened [and 1 more messages]

[PATCH v3] xenconsole: adjust pty opening error checking and handling