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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 01/12/2013 23:14, Matthew Daley wrote:
> On Mon, Dec 2, 2013 at 12:41 AM, Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx> 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@xxxxxxxxxxx>
>>> ---
>>>  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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel