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

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

[Matthew Daley]

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

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

[Andrew Cooper]

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);
>  			}
>  		}

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

[Matthew Daley]

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

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

[Andrew Cooper]

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

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

[Matthew Daley]

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

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

[Andrew Cooper]

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);
>  			}
>  		}

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

[Ian Jackson]

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.

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

[Andrew Cooper]

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

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

[Matthew Daley]

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
>

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

[Ian Jackson]

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.

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

[Matthew Daley]

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.

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

[Matthew Daley]

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

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

[Ian Jackson]

Matthew Daley writes ("[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.

Thanks, and sorry to have made such a meal of this.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>