Re: [PATCH xf86-input-libinput] Fix crash when using threaded input and the first device goes away

2016-10-10 Thread Hans de Goede 

Hi,

On 10/10/2016 11:10 AM, Emil Velikov wrote:

Hi Hans,

On Wednesday, 5 October 2016, Hans de Goede > wrote:

When the xserver uses threaded input, it keeps a pointer to the InputInfo
passed into xf86AddEnabledDevice and calls pointer->read_input on events.

But when the first enabled device goes away the pInfo we've passed into
xf86AddEnabledDevice gets freed and eventually pInfo->read_input gets
overwritten (or pInfo points to unmapped memory) leading to a segfault.

This commit fixes this by replacing the pInfo passed into
xf86AddEnabledDevice with a pointer to a global InputInfo stored inside
the driver_context struct.

Signed-off-by: Hans de Goede 
---
 src/xf86libinput.c | 22 --
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/src/xf86libinput.c b/src/xf86libinput.c
index 21f87f5..485e212 100644
--- a/src/xf86libinput.c
+++ b/src/xf86libinput.c
@@ -86,6 +86,7 @@

 struct xf86libinput_driver {
struct libinput *libinput;
+   struct _InputInfoRec InputInfo;
int device_enabled_count;
 };

@@ -582,7 +583,17 @@ xf86libinput_on(DeviceIntPtr dev)

if (driver_context.device_enabled_count == 0) {
 #if HAVE_THREADED_INPUT
-   xf86AddEnabledDevice(pInfo);
+   /*
+* The xserver keeps a pointer to the InputInfo passed into
+* xf86AddEnabledDevice and calls pointer->read_input on
+* events. Thus we cannot simply pass in our current pInfo
+* as that will be deleted when the current input device 
gets
+* unplugged. Instead pass in a pointer to a global
+* InputInfo inside the driver_context.
+*/
+   driver_context.InputInfo.fd = pInfo->fd;

Reading the above comment makes me wonder about the lifetime of the fd. I take 
it that we're not leaking it atm ?


The fd here actually is libinput's epoll fd, which is shared by all the devices
and gets closed when we destroy the libinput context which we do already when 
the
last libinput driven device gets removed.


+   driver_context.InputInfo.read_input = pInfo->read_input;
+   xf86AddEnabledDevice(_context.InputInfo);
 #else
/* Can't use xf86AddEnabledDevice on an epollfd */
AddEnabledDevice(pInfo->fd);

Can we use the same storage for the !HAVE_THREADED_INPUT code paths ?


I've not looked closely at the !threaded code paths, but AFAIK the non threaded
paths take just a fd; and then later lookup the pInfo in the list of devices,
so in that case the pInfo must be a real pInfo.




@@ -606,7 +617,7 @@ xf86libinput_off(DeviceIntPtr dev)

if (--driver_context.device_enabled_count == 0) {
 #if HAVE_THREADED_INPUT
-   xf86RemoveEnabledDevice(pInfo);
+   xf86RemoveEnabledDevice(_context.InputInfo);
 #else
RemoveEnabledDevice(pInfo->fd);
 #endif
@@ -1923,7 +1934,7 @@ out:
 }

 static void
-xf86libinput_read_input(InputInfoPtr pInfo)
+xf86libinput_read_input(InputInfoPtr do_not_use)

Worth just dropping the argument and fixing the caller(s)?


This is a callback function called by the server, so we cannot just
change the signature; and other input drivers are likely to actually
use the argument.

Regards,

Hans
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xf86-input-libinput] Fix crash when using threaded input and the first device goes away

2016-10-10 Thread Emil Velikov 
Hi Hans,

On Wednesday, 5 October 2016, Hans de Goede  wrote:

> When the xserver uses threaded input, it keeps a pointer to the InputInfo
> passed into xf86AddEnabledDevice and calls pointer->read_input on events.
>
> But when the first enabled device goes away the pInfo we've passed into
> xf86AddEnabledDevice gets freed and eventually pInfo->read_input gets
> overwritten (or pInfo points to unmapped memory) leading to a segfault.
>
> This commit fixes this by replacing the pInfo passed into
> xf86AddEnabledDevice with a pointer to a global InputInfo stored inside
> the driver_context struct.
>
> Signed-off-by: Hans de Goede >
> ---
>  src/xf86libinput.c | 22 --
>  1 file changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/src/xf86libinput.c b/src/xf86libinput.c
> index 21f87f5..485e212 100644
> --- a/src/xf86libinput.c
> +++ b/src/xf86libinput.c
> @@ -86,6 +86,7 @@
>
>  struct xf86libinput_driver {
> struct libinput *libinput;
> +   struct _InputInfoRec InputInfo;
> int device_enabled_count;
>  };
>
> @@ -582,7 +583,17 @@ xf86libinput_on(DeviceIntPtr dev)
>
> if (driver_context.device_enabled_count == 0) {
>  #if HAVE_THREADED_INPUT
> -   xf86AddEnabledDevice(pInfo);
> +   /*
> +* The xserver keeps a pointer to the InputInfo passed into
> +* xf86AddEnabledDevice and calls pointer->read_input on
> +* events. Thus we cannot simply pass in our current pInfo
> +* as that will be deleted when the current input device
> gets
> +* unplugged. Instead pass in a pointer to a global
> +* InputInfo inside the driver_context.
> +*/
> +   driver_context.InputInfo.fd = pInfo->fd;

Reading the above comment makes me wonder about the lifetime of the fd. I
take it that we're not leaking it atm ?


> +   driver_context.InputInfo.read_input = pInfo->read_input;
> +   xf86AddEnabledDevice(_context.InputInfo);
>  #else
> /* Can't use xf86AddEnabledDevice on an epollfd */
> AddEnabledDevice(pInfo->fd);

Can we use the same storage for the !HAVE_THREADED_INPUT code paths ?


> @@ -606,7 +617,7 @@ xf86libinput_off(DeviceIntPtr dev)
>
> if (--driver_context.device_enabled_count == 0) {
>  #if HAVE_THREADED_INPUT
> -   xf86RemoveEnabledDevice(pInfo);
> +   xf86RemoveEnabledDevice(_context.InputInfo);
>  #else
> RemoveEnabledDevice(pInfo->fd);
>  #endif
> @@ -1923,7 +1934,7 @@ out:
>  }
>
>  static void
> -xf86libinput_read_input(InputInfoPtr pInfo)
> +xf86libinput_read_input(InputInfoPtr do_not_use)

Worth just dropping the argument and fixing the caller(s)?

Emil
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xf86-input-libinput] Fix crash when using threaded input and the first device goes away

2016-10-05 Thread Hans de Goede 

Hi,

On 05-10-16 15:31, Hans de Goede wrote:

When the xserver uses threaded input, it keeps a pointer to the InputInfo
passed into xf86AddEnabledDevice and calls pointer->read_input on events.

But when the first enabled device goes away the pInfo we've passed into
xf86AddEnabledDevice gets freed and eventually pInfo->read_input gets
overwritten (or pInfo points to unmapped memory) leading to a segfault.

This commit fixes this by replacing the pInfo passed into
xf86AddEnabledDevice with a pointer to a global InputInfo stored inside
the driver_context struct.

Signed-off-by: Hans de Goede 


Forgot to add a buglink, this fixes:

https://bugzilla.redhat.com/show_bug.cgi?id=1381840

Regards,

Hans



---
 src/xf86libinput.c | 22 --
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/src/xf86libinput.c b/src/xf86libinput.c
index 21f87f5..485e212 100644
--- a/src/xf86libinput.c
+++ b/src/xf86libinput.c
@@ -86,6 +86,7 @@

 struct xf86libinput_driver {
struct libinput *libinput;
+   struct _InputInfoRec InputInfo;
int device_enabled_count;
 };

@@ -582,7 +583,17 @@ xf86libinput_on(DeviceIntPtr dev)

if (driver_context.device_enabled_count == 0) {
 #if HAVE_THREADED_INPUT
-   xf86AddEnabledDevice(pInfo);
+   /*
+* The xserver keeps a pointer to the InputInfo passed into
+* xf86AddEnabledDevice and calls pointer->read_input on
+* events. Thus we cannot simply pass in our current pInfo
+* as that will be deleted when the current input device gets
+* unplugged. Instead pass in a pointer to a global
+* InputInfo inside the driver_context.
+*/
+   driver_context.InputInfo.fd = pInfo->fd;
+   driver_context.InputInfo.read_input = pInfo->read_input;
+   xf86AddEnabledDevice(_context.InputInfo);
 #else
/* Can't use xf86AddEnabledDevice on an epollfd */
AddEnabledDevice(pInfo->fd);
@@ -606,7 +617,7 @@ xf86libinput_off(DeviceIntPtr dev)

if (--driver_context.device_enabled_count == 0) {
 #if HAVE_THREADED_INPUT
-   xf86RemoveEnabledDevice(pInfo);
+   xf86RemoveEnabledDevice(_context.InputInfo);
 #else
RemoveEnabledDevice(pInfo->fd);
 #endif
@@ -1923,7 +1934,7 @@ out:
 }

 static void
-xf86libinput_read_input(InputInfoPtr pInfo)
+xf86libinput_read_input(InputInfoPtr do_not_use)
 {
struct libinput *libinput = driver_context.libinput;
int rc;
@@ -1934,9 +1945,8 @@ xf86libinput_read_input(InputInfoPtr pInfo)
return;

if (rc < 0) {
-   xf86IDrvMsg(pInfo, X_ERROR,
-   "Error reading events: %s\n",
-   strerror(-rc));
+   xf86Msg(X_ERROR, "Error reading libinput events: %s\n",
+   strerror(-rc));
return;
}



___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH xf86-input-libinput] Fix crash when using threaded input and the first device goes away

2016-10-05 Thread Hans de Goede 
When the xserver uses threaded input, it keeps a pointer to the InputInfo
passed into xf86AddEnabledDevice and calls pointer->read_input on events.

But when the first enabled device goes away the pInfo we've passed into
xf86AddEnabledDevice gets freed and eventually pInfo->read_input gets
overwritten (or pInfo points to unmapped memory) leading to a segfault.

This commit fixes this by replacing the pInfo passed into
xf86AddEnabledDevice with a pointer to a global InputInfo stored inside
the driver_context struct.

Signed-off-by: Hans de Goede 
---
 src/xf86libinput.c | 22 --
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/src/xf86libinput.c b/src/xf86libinput.c
index 21f87f5..485e212 100644
--- a/src/xf86libinput.c
+++ b/src/xf86libinput.c
@@ -86,6 +86,7 @@
 
 struct xf86libinput_driver {
struct libinput *libinput;
+   struct _InputInfoRec InputInfo;
int device_enabled_count;
 };
 
@@ -582,7 +583,17 @@ xf86libinput_on(DeviceIntPtr dev)
 
if (driver_context.device_enabled_count == 0) {
 #if HAVE_THREADED_INPUT
-   xf86AddEnabledDevice(pInfo);
+   /*
+* The xserver keeps a pointer to the InputInfo passed into
+* xf86AddEnabledDevice and calls pointer->read_input on
+* events. Thus we cannot simply pass in our current pInfo
+* as that will be deleted when the current input device gets
+* unplugged. Instead pass in a pointer to a global
+* InputInfo inside the driver_context.
+*/
+   driver_context.InputInfo.fd = pInfo->fd;
+   driver_context.InputInfo.read_input = pInfo->read_input;
+   xf86AddEnabledDevice(_context.InputInfo);
 #else
/* Can't use xf86AddEnabledDevice on an epollfd */
AddEnabledDevice(pInfo->fd);
@@ -606,7 +617,7 @@ xf86libinput_off(DeviceIntPtr dev)
 
if (--driver_context.device_enabled_count == 0) {
 #if HAVE_THREADED_INPUT
-   xf86RemoveEnabledDevice(pInfo);
+   xf86RemoveEnabledDevice(_context.InputInfo);
 #else
RemoveEnabledDevice(pInfo->fd);
 #endif
@@ -1923,7 +1934,7 @@ out:
 }
 
 static void
-xf86libinput_read_input(InputInfoPtr pInfo)
+xf86libinput_read_input(InputInfoPtr do_not_use)
 {
struct libinput *libinput = driver_context.libinput;
int rc;
@@ -1934,9 +1945,8 @@ xf86libinput_read_input(InputInfoPtr pInfo)
return;
 
if (rc < 0) {
-   xf86IDrvMsg(pInfo, X_ERROR,
-   "Error reading events: %s\n",
-   strerror(-rc));
+   xf86Msg(X_ERROR, "Error reading libinput events: %s\n",
+   strerror(-rc));
return;
}
 
-- 
2.9.3

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel