[libxcb] Introduce new funcion xcb_connect_with_auth_file().

Submitted by Laércio de Sousa on Oct. 17, 2014, 6:23 p.m.

Details

Message ID 1413570228-8416-1-git-send-email-laerciosousa@sme-mogidascruzes.sp.gov.br
State New
Headers show

Commit Message

Laércio de Sousa Oct. 17, 2014, 6:23 p.m.
This patch introduces a function called xcb_connect_with_auth_file(),
which is similar to xcb_connect_to_display_with_auth_info(), but expects
an authorization file path rather than a xcb_auth_info_t struct.

Signed-off-by: Laércio de Sousa <laerciosousa@sme-mogidascruzes.sp.gov.br>
---
 src/xcb.h      | 21 +++++++++++++++++++++
 src/xcb_auth.c | 15 +++++++++++++--
 src/xcb_util.c | 27 ++++++++++++++++++++-------
 src/xcbint.h   |  2 +-
 4 files changed, 55 insertions(+), 10 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/xcb.h b/src/xcb.h
index 23fe74e..0314ce5 100644
--- a/src/xcb.h
+++ b/src/xcb.h
@@ -535,6 +535,27 @@  int xcb_parse_display(const char *name, char **host, int *display, int *screen);
 xcb_connection_t *xcb_connect(const char *displayname, int *screenp);
 
 /**
+ * @brief Connects to the X server, using and authorization file.
+ * @param displayname: The name of the display.
+ * @param authfile: The authorization file path.
+ * @param screenp: A pointer to a preferred screen number.
+ * @return A newly allocated xcb_connection_t structure.
+ *
+ * Connects to the X server specified by @p displayname, using the
+ * authorization file @p authfile. If @p authfile value @c NULL, uses
+ * the value of the XAUTHORITY environment variable. If a particular
+ * screen on that server is preferred, the int pointed to by @p screenp
+ * (if not @c NULL) will be set to that screen; otherwise @p screenp
+ * will be set to 0.
+ *
+ * Always returns a non-NULL pointer to a xcb_connection_t, even on failure.
+ * Callers need to use xcb_connection_has_error() to check for failure.
+ * When finished, use xcb_disconnect() to close the connection and free
+ * the structure.
+ */
+xcb_connection_t *xcb_connect_with_auth_file(const char *displayname, const char *authfile, int *screenp);
+
+/**
  * @brief Connects to the X server, using an authorization information.
  * @param display: The name of the display.
  * @param auth: The authorization information.
diff --git a/src/xcb_auth.c b/src/xcb_auth.c
index 29e2b6f..284a582 100644
--- a/src/xcb_auth.c
+++ b/src/xcb_auth.c
@@ -34,6 +34,7 @@ 
 #include <sys/param.h>
 #include <unistd.h>
 #include <stdlib.h>
+#include <stdio.h>
 
 #ifdef __INTERIX
 /* _don't_ ask. interix has INADDR_LOOPBACK in here. */
@@ -309,7 +310,7 @@  static struct sockaddr *get_peer_sock_name(int (*socket_func)(int,
     return NULL;
 }
 
-int _xcb_get_auth_info(int fd, xcb_auth_info_t *info, int display)
+int _xcb_get_auth_info(int fd, xcb_auth_info_t *info, int display, const char *authfile)
 {
     /* code adapted from Xlib/ConnDis.c, xtrans/Xtranssocket.c,
        xtrans/Xtransutils.c */
@@ -334,7 +335,17 @@  int _xcb_get_auth_info(int fd, xcb_auth_info_t *info, int display)
         gotsockname = 1;
     }
 
-    authptr = get_authptr(sockname, display);
+    if (authfile) {
+        FILE *f = fopen(authfile, "r");
+
+        if (f) {
+            authptr = XauReadAuth(f);
+            fclose(f);
+        }
+    }
+    else
+        authptr = get_authptr(sockname, display);
+
     if (authptr == 0)
     {
         free(sockname);
diff --git a/src/xcb_util.c b/src/xcb_util.c
index ba0f108..aafb73d 100644
--- a/src/xcb_util.c
+++ b/src/xcb_util.c
@@ -475,12 +475,10 @@  static int _xcb_open_abstract(char *protocol, const char *file, size_t filelen)
 }
 #endif
 
-xcb_connection_t *xcb_connect(const char *displayname, int *screenp)
-{
-    return xcb_connect_to_display_with_auth_info(displayname, NULL, screenp);
-}
-
-xcb_connection_t *xcb_connect_to_display_with_auth_info(const char *displayname, xcb_auth_info_t *auth, int *screenp)
+static xcb_connection_t *_xcb_connect_to_display_with_auth_info_or_file(const char *displayname,
+                                                                        const char *authfile,
+                                                                        xcb_auth_info_t *auth,
+                                                                        int *screenp)
 {
     int fd, display = 0;
     char *host = NULL;
@@ -518,7 +516,7 @@  xcb_connection_t *xcb_connect_to_display_with_auth_info(const char *displayname,
         goto out;
     }
 
-    if(_xcb_get_auth_info(fd, &ourauth, display))
+    if(_xcb_get_auth_info(fd, &ourauth, display, authfile))
     {
         c = xcb_connect_to_fd(fd, &ourauth);
         free(ourauth.name);
@@ -542,3 +540,18 @@  out:
     free(protocol);
     return c;
 }
+
+xcb_connection_t *xcb_connect(const char *displayname, int *screenp)
+{
+    return xcb_connect_to_display_with_auth_info(displayname, NULL, screenp);
+}
+
+xcb_connection_t *xcb_connect_with_auth_file(const char *displayname, const char *authfile, int *screenp)
+{
+    return _xcb_connect_to_display_with_auth_info_or_file(displayname, authfile, NULL, screenp);
+}
+
+xcb_connection_t *xcb_connect_to_display_with_auth_info(const char *displayname, xcb_auth_info_t *auth, int *screenp)
+{
+    return _xcb_connect_to_display_with_auth_info_or_file(displayname, NULL, auth, screenp);
+}
diff --git a/src/xcbint.h b/src/xcbint.h
index f89deba..01aca8c 100644
--- a/src/xcbint.h
+++ b/src/xcbint.h
@@ -218,7 +218,7 @@  int _xcb_conn_wait(xcb_connection_t *c, pthread_cond_t *cond, struct iovec **vec
 
 /* xcb_auth.c */
 
-int _xcb_get_auth_info(int fd, xcb_auth_info_t *info, int display);
+int _xcb_get_auth_info(int fd, xcb_auth_info_t *info, int display, const char *authfile);
 
 #ifdef GCC_HAS_VISIBILITY
 #pragma GCC visibility pop

Comments

Uli Schlachter Oct. 18, 2014, 8:17 a.m.
Hi,

typo in the subject: funcTion

I have not much clue about X11 authentication. Could someone else comment on the
idea behind this patch?

Am 17.10.2014 um 20:23 schrieb Laércio de Sousa:
> This patch introduces a function called xcb_connect_with_auth_file(),
> which is similar to xcb_connect_to_display_with_auth_info(), but expects
> an authorization file path rather than a xcb_auth_info_t struct.
> 
> Signed-off-by: Laércio de Sousa <laerciosousa@sme-mogidascruzes.sp.gov.br>
> ---
>  src/xcb.h      | 21 +++++++++++++++++++++
>  src/xcb_auth.c | 15 +++++++++++++--
>  src/xcb_util.c | 27 ++++++++++++++++++++-------
>  src/xcbint.h   |  2 +-
>  4 files changed, 55 insertions(+), 10 deletions(-)
> 
> diff --git a/src/xcb.h b/src/xcb.h
> index 23fe74e..0314ce5 100644
> --- a/src/xcb.h
> +++ b/src/xcb.h
> @@ -535,6 +535,27 @@ int xcb_parse_display(const char *name, char **host, int *display, int *screen);
>  xcb_connection_t *xcb_connect(const char *displayname, int *screenp);
>  
>  /**
> + * @brief Connects to the X server, using and authorization file.
> + * @param displayname: The name of the display.
> + * @param authfile: The authorization file path.
> + * @param screenp: A pointer to a preferred screen number.
> + * @return A newly allocated xcb_connection_t structure.
> + *
> + * Connects to the X server specified by @p displayname, using the
> + * authorization file @p authfile. If @p authfile value @c NULL, uses
> + * the value of the XAUTHORITY environment variable. If a particular
> + * screen on that server is preferred, the int pointed to by @p screenp
> + * (if not @c NULL) will be set to that screen; otherwise @p screenp
> + * will be set to 0.
> + *
> + * Always returns a non-NULL pointer to a xcb_connection_t, even on failure.
> + * Callers need to use xcb_connection_has_error() to check for failure.
> + * When finished, use xcb_disconnect() to close the connection and free
> + * the structure.
> + */
> +xcb_connection_t *xcb_connect_with_auth_file(const char *displayname, const char *authfile, int *screenp);
> +
> +/**
>   * @brief Connects to the X server, using an authorization information.
>   * @param display: The name of the display.
>   * @param auth: The authorization information.
> diff --git a/src/xcb_auth.c b/src/xcb_auth.c
> index 29e2b6f..284a582 100644
> --- a/src/xcb_auth.c
> +++ b/src/xcb_auth.c
> @@ -34,6 +34,7 @@
>  #include <sys/param.h>
>  #include <unistd.h>
>  #include <stdlib.h>
> +#include <stdio.h>
>  
>  #ifdef __INTERIX
>  /* _don't_ ask. interix has INADDR_LOOPBACK in here. */
> @@ -309,7 +310,7 @@ static struct sockaddr *get_peer_sock_name(int (*socket_func)(int,
>      return NULL;
>  }
>  
> -int _xcb_get_auth_info(int fd, xcb_auth_info_t *info, int display)
> +int _xcb_get_auth_info(int fd, xcb_auth_info_t *info, int display, const char *authfile)
>  {
>      /* code adapted from Xlib/ConnDis.c, xtrans/Xtranssocket.c,
>         xtrans/Xtransutils.c */
> @@ -334,7 +335,17 @@ int _xcb_get_auth_info(int fd, xcb_auth_info_t *info, int display)
>          gotsockname = 1;
>      }
>  
> -    authptr = get_authptr(sockname, display);
> +    if (authfile) {
> +        FILE *f = fopen(authfile, "r");
> +
> +        if (f) {

Instead of ignoring errors, shouldn't this return an error connection to the
caller?

> +            authptr = XauReadAuth(f);

Ok, now I actually googled this code and the man page says:

   XauReadAuth reads the next entry from  auth_file.   The  entry  is  not
   statically allocated and should be freed by calling XauDisposeAuth.

I have no clue about Xau, but the part that says "the next entry" makes me think
that this code doesn't actually work well. And yeah, "xauth list" in a terminal
obviously indicates that an XAUTHORITY file can contain multiple
authentications. Your code will only use the first one.

> +            fclose(f);
> +        }
> +    }
> +    else
> +        authptr = get_authptr(sockname, display);
> +
>      if (authptr == 0)
>      {
>          free(sockname);
> diff --git a/src/xcb_util.c b/src/xcb_util.c
> index ba0f108..aafb73d 100644
> --- a/src/xcb_util.c
> +++ b/src/xcb_util.c
> @@ -475,12 +475,10 @@ static int _xcb_open_abstract(char *protocol, const char *file, size_t filelen)
>  }
>  #endif
>  
> -xcb_connection_t *xcb_connect(const char *displayname, int *screenp)
> -{
> -    return xcb_connect_to_display_with_auth_info(displayname, NULL, screenp);
> -}
> -
> -xcb_connection_t *xcb_connect_to_display_with_auth_info(const char *displayname, xcb_auth_info_t *auth, int *screenp)
> +static xcb_connection_t *_xcb_connect_to_display_with_auth_info_or_file(const char *displayname,
> +                                                                        const char *authfile,
> +                                                                        xcb_auth_info_t *auth,
> +                                                                        int *screenp)
>  {
>      int fd, display = 0;
>      char *host = NULL;
> @@ -518,7 +516,7 @@ xcb_connection_t *xcb_connect_to_display_with_auth_info(const char *displayname,
>          goto out;
>      }
>  
> -    if(_xcb_get_auth_info(fd, &ourauth, display))
> +    if(_xcb_get_auth_info(fd, &ourauth, display, authfile))
>      {
>          c = xcb_connect_to_fd(fd, &ourauth);
>          free(ourauth.name);
> @@ -542,3 +540,18 @@ out:
>      free(protocol);
>      return c;
>  }
> +
> +xcb_connection_t *xcb_connect(const char *displayname, int *screenp)
> +{
> +    return xcb_connect_to_display_with_auth_info(displayname, NULL, screenp);

I'd propose to call _xcb_connect_to_display_with_auth_info_or_file() directly
(urgh, what a function name...).

> +}
> +
> +xcb_connection_t *xcb_connect_with_auth_file(const char *displayname, const char *authfile, int *screenp)
> +{
> +    return _xcb_connect_to_display_with_auth_info_or_file(displayname, authfile, NULL, screenp);
> +}
> +
> +xcb_connection_t *xcb_connect_to_display_with_auth_info(const char *displayname, xcb_auth_info_t *auth, int *screenp)
> +{
> +    return _xcb_connect_to_display_with_auth_info_or_file(displayname, NULL, auth, screenp);
> +}
> diff --git a/src/xcbint.h b/src/xcbint.h
> index f89deba..01aca8c 100644
> --- a/src/xcbint.h
> +++ b/src/xcbint.h
> @@ -218,7 +218,7 @@ int _xcb_conn_wait(xcb_connection_t *c, pthread_cond_t *cond, struct iovec **vec
>  
>  /* xcb_auth.c */
>  
> -int _xcb_get_auth_info(int fd, xcb_auth_info_t *info, int display);
> +int _xcb_get_auth_info(int fd, xcb_auth_info_t *info, int display, const char *authfile);
>  
>  #ifdef GCC_HAS_VISIBILITY
>  #pragma GCC visibility pop
> 

Cheers,
Uli
Uli Schlachter Oct. 18, 2014, 8:34 a.m.
Am 17.10.2014 um 20:23 schrieb Laércio de Sousa:
[...]
> @@ -334,7 +335,17 @@ int _xcb_get_auth_info(int fd, xcb_auth_info_t *info, int display)
>          gotsockname = 1;
>      }
>  
> -    authptr = get_authptr(sockname, display);
> +    if (authfile) {
> +        FILE *f = fopen(authfile, "r");
[...]

Thanks to Windows, this should be "rb" instead of "r" (I was reading through Xau
source code and noticed this).

Uli
Laércio de Sousa Oct. 20, 2014, 11:06 a.m.
2014-10-18 5:17 GMT-03:00 Uli Schlachter <psychon@znc.in>:

> Hi,
>
> typo in the subject: funcTion
>

Fixed. Thanks!


> I have not much clue about X11 authentication. Could someone else comment
> on the
> idea behind this patch?
>
> Am 17.10.2014 um 20:23 schrieb Laércio de Sousa:
> > This patch introduces a function called xcb_connect_with_auth_file(),
> > which is similar to xcb_connect_to_display_with_auth_info(), but expects
> > an authorization file path rather than a xcb_auth_info_t struct.
> >
> > Signed-off-by: Laércio de Sousa <
> laerciosousa@sme-mogidascruzes.sp.gov.br>
> > ---
> >  src/xcb.h      | 21 +++++++++++++++++++++
> >  src/xcb_auth.c | 15 +++++++++++++--
> >  src/xcb_util.c | 27 ++++++++++++++++++++-------
> >  src/xcbint.h   |  2 +-
> >  4 files changed, 55 insertions(+), 10 deletions(-)
> >
> > diff --git a/src/xcb.h b/src/xcb.h
> > index 23fe74e..0314ce5 100644
> > --- a/src/xcb.h
> > +++ b/src/xcb.h
> > @@ -535,6 +535,27 @@ int xcb_parse_display(const char *name, char
> **host, int *display, int *screen);
> >  xcb_connection_t *xcb_connect(const char *displayname, int *screenp);
> >
> >  /**
> > + * @brief Connects to the X server, using and authorization file.
> > + * @param displayname: The name of the display.
> > + * @param authfile: The authorization file path.
> > + * @param screenp: A pointer to a preferred screen number.
> > + * @return A newly allocated xcb_connection_t structure.
> > + *
> > + * Connects to the X server specified by @p displayname, using the
> > + * authorization file @p authfile. If @p authfile value @c NULL, uses
> > + * the value of the XAUTHORITY environment variable. If a particular
> > + * screen on that server is preferred, the int pointed to by @p screenp
> > + * (if not @c NULL) will be set to that screen; otherwise @p screenp
> > + * will be set to 0.
> > + *
> > + * Always returns a non-NULL pointer to a xcb_connection_t, even on
> failure.
> > + * Callers need to use xcb_connection_has_error() to check for failure.
> > + * When finished, use xcb_disconnect() to close the connection and free
> > + * the structure.
> > + */
> > +xcb_connection_t *xcb_connect_with_auth_file(const char *displayname,
> const char *authfile, int *screenp);
> > +
> > +/**
> >   * @brief Connects to the X server, using an authorization information.
> >   * @param display: The name of the display.
> >   * @param auth: The authorization information.
> > diff --git a/src/xcb_auth.c b/src/xcb_auth.c
> > index 29e2b6f..284a582 100644
> > --- a/src/xcb_auth.c
> > +++ b/src/xcb_auth.c
> > @@ -34,6 +34,7 @@
> >  #include <sys/param.h>
> >  #include <unistd.h>
> >  #include <stdlib.h>
> > +#include <stdio.h>
> >
> >  #ifdef __INTERIX
> >  /* _don't_ ask. interix has INADDR_LOOPBACK in here. */
> > @@ -309,7 +310,7 @@ static struct sockaddr *get_peer_sock_name(int
> (*socket_func)(int,
> >      return NULL;
> >  }
> >
> > -int _xcb_get_auth_info(int fd, xcb_auth_info_t *info, int display)
> > +int _xcb_get_auth_info(int fd, xcb_auth_info_t *info, int display,
> const char *authfile)
> >  {
> >      /* code adapted from Xlib/ConnDis.c, xtrans/Xtranssocket.c,
> >         xtrans/Xtransutils.c */
> > @@ -334,7 +335,17 @@ int _xcb_get_auth_info(int fd, xcb_auth_info_t
> *info, int display)
> >          gotsockname = 1;
> >      }
> >
> > -    authptr = get_authptr(sockname, display);
> > +    if (authfile) {
> > +        FILE *f = fopen(authfile, "r");
> > +
> > +        if (f) {
>
> Instead of ignoring errors, shouldn't this return an error connection to
> the
> caller?
>
> > +            authptr = XauReadAuth(f);
>
> Ok, now I actually googled this code and the man page says:
>
>    XauReadAuth reads the next entry from  auth_file.   The  entry  is  not
>    statically allocated and should be freed by calling XauDisposeAuth.
>
> I have no clue about Xau, but the part that says "the next entry" makes me
> think
> that this code doesn't actually work well. And yeah, "xauth list" in a
> terminal
> obviously indicates that an XAUTHORITY file can contain multiple
> authentications. Your code will only use the first one.
>

I'm thinking about this. libxcb currently calls XauGetBestAuthByAddr(),
which calls XauReadAuth() on the file name returned by XauFileName() ---
namely, the value of XAUTHORITY environment variable or ~/.Xauthority.
Maybe the best option would be patching libXau so we can pass explicitly a
file name path to XauGetBestAuthByAddr() --- if none is passed, get one by
calling XauFileName().

I'm affraid all this is becoming too complex. Maybe I should just give up
and call setenv("XAUTHORITY", ...) from my client application before
calling xcb_connect(). My main motivation for proposing this patch is about
symmetry: if I can call xcb_connect() with an explicit display number ---
instead of calling setenv("DISPLAY", ...) and then xcb_connect(NULL,...)
--- shouldn't I be able to do the same with the authorization file?

> +            fclose(f);
> > +        }
> > +    }
> > +    else
> > +        authptr = get_authptr(sockname, display);
> > +
> >      if (authptr == 0)
> >      {
> >          free(sockname);
> > diff --git a/src/xcb_util.c b/src/xcb_util.c
> > index ba0f108..aafb73d 100644
> > --- a/src/xcb_util.c
> > +++ b/src/xcb_util.c
> > @@ -475,12 +475,10 @@ static int _xcb_open_abstract(char *protocol,
> const char *file, size_t filelen)
> >  }
> >  #endif
> >
> > -xcb_connection_t *xcb_connect(const char *displayname, int *screenp)
> > -{
> > -    return xcb_connect_to_display_with_auth_info(displayname, NULL,
> screenp);
> > -}
> > -
> > -xcb_connection_t *xcb_connect_to_display_with_auth_info(const char
> *displayname, xcb_auth_info_t *auth, int *screenp)
> > +static xcb_connection_t
> *_xcb_connect_to_display_with_auth_info_or_file(const char *displayname,
> > +
> const char *authfile,
> > +
> xcb_auth_info_t *auth,
> > +
> int *screenp)
> >  {
> >      int fd, display = 0;
> >      char *host = NULL;
> > @@ -518,7 +516,7 @@ xcb_connection_t
> *xcb_connect_to_display_with_auth_info(const char *displayname,
> >          goto out;
> >      }
> >
> > -    if(_xcb_get_auth_info(fd, &ourauth, display))
> > +    if(_xcb_get_auth_info(fd, &ourauth, display, authfile))
> >      {
> >          c = xcb_connect_to_fd(fd, &ourauth);
> >          free(ourauth.name);
> > @@ -542,3 +540,18 @@ out:
> >      free(protocol);
> >      return c;
> >  }
> > +
> > +xcb_connection_t *xcb_connect(const char *displayname, int *screenp)
> > +{
> > +    return xcb_connect_to_display_with_auth_info(displayname, NULL,
> screenp);
>
> I'd propose to call _xcb_connect_to_display_with_auth_info_or_file()
> directly
>

Fixed. Thanks!


> (urgh, what a function name...).
>

I've renamed it to _xcb_connect_to_display_with_auth(). Does it look better
now? :-)


>
> > +}
> > +
> > +xcb_connection_t *xcb_connect_with_auth_file(const char *displayname,
> const char *authfile, int *screenp)
> > +{
> > +    return _xcb_connect_to_display_with_auth_info_or_file(displayname,
> authfile, NULL, screenp);
> > +}
> > +
> > +xcb_connection_t *xcb_connect_to_display_with_auth_info(const char
> *displayname, xcb_auth_info_t *auth, int *screenp)
> > +{
> > +    return _xcb_connect_to_display_with_auth_info_or_file(displayname,
> NULL, auth, screenp);
> > +}
> > diff --git a/src/xcbint.h b/src/xcbint.h
> > index f89deba..01aca8c 100644
> > --- a/src/xcbint.h
> > +++ b/src/xcbint.h
> > @@ -218,7 +218,7 @@ int _xcb_conn_wait(xcb_connection_t *c,
> pthread_cond_t *cond, struct iovec **vec
> >
> >  /* xcb_auth.c */
> >
> > -int _xcb_get_auth_info(int fd, xcb_auth_info_t *info, int display);
> > +int _xcb_get_auth_info(int fd, xcb_auth_info_t *info, int display,
> const char *authfile);
> >
> >  #ifdef GCC_HAS_VISIBILITY
> >  #pragma GCC visibility pop
> >
>
> Cheers,
> Uli
> --
> A normal person is just someone you don't know well enough yet.
>  - Nettie Wiebe
>
Laércio de Sousa Oct. 20, 2014, 11:24 a.m.
2014-10-18 5:34 GMT-03:00 Uli Schlachter <psychon@znc.in>:

> Am 17.10.2014 um 20:23 schrieb Laércio de Sousa:
> [...]
> > @@ -334,7 +335,17 @@ int _xcb_get_auth_info(int fd, xcb_auth_info_t
> *info, int display)
> >          gotsockname = 1;
> >      }
> >
> > -    authptr = get_authptr(sockname, display);
> > +    if (authfile) {
> > +        FILE *f = fopen(authfile, "r");
> [...]
>
> Thanks to Windows, this should be "rb" instead of "r" (I was reading
> through Xau
> source code and noticed this).
>

FIxed. Thanks!

Uli
> --
> A normal person is just someone you don't know well enough yet.
>  - Nettie Wiebe
>