Update: [PATCH 2/6] xserver: Possible memory leaks, stricter option checks, UnInit (NewInputDeviceRequest)

[Magnus Vigerlöf]

Possible memory leaks, stricter option checks, UnInit introduction

Added more checks on the options, specifically for the driver and 
name/identifier options. Added unwind section to handle fault cases.

--
Misplaced first check of if the driver was already loaded. (xf86LookupInputDriver)

 hw/xfree86/common/xf86Helper.c |    5 ++-
 hw/xfree86/common/xf86Xinput.c |   68 +++++++++++++++++++++++++++++++---------
 2 files changed, 56 insertions(+), 17 deletions(-)
--
diff --git a/hw/xfree86/common/xf86Helper.c b/hw/xfree86/common/xf86Helper.c
index 913735b..cfd483f 100644
--- a/hw/xfree86/common/xf86Helper.c
+++ b/hw/xfree86/common/xf86Helper.c
@@ -371,8 +371,9 @@ #endif
     if (pInp->drv)
 	pInp->drv->refCount--;
 
-    if (pInp->private)
-	xfree(pInp->private);
+    /* This should *really* be handled in drv->UnInit(dev) call instead */
+/*    if (pInp->private)
+	xfree(pInp->private);*/
 
     /* Remove the entry from the list. */
     if (pInp == xf86InputDevs)
diff --git a/hw/xfree86/common/xf86Xinput.c b/hw/xfree86/common/xf86Xinput.c
index 17ffed8..3800d9a 100644
--- a/hw/xfree86/common/xf86Xinput.c
+++ b/hw/xfree86/common/xf86Xinput.c
@@ -322,6 +322,7 @@ NewInputDeviceRequest (InputOption *opti
     InputInfoPtr pInfo = NULL;
     InputOption *option = NULL;
     DeviceIntPtr dev = NULL;
+    int rval = Success;
 
     idev = xcalloc(sizeof(*idev), 1);
     if (!idev)
@@ -329,64 +330,101 @@ NewInputDeviceRequest (InputOption *opti
 
     for (option = options; option; option = option->next) {
         if (strcmp(option->key, "driver") == 0) {
-            if (!xf86LoadOneModule(option->value, NULL))
-                return BadName;
+            if (idev->driver) {
+                rval = BadRequest;
+                goto unwind;
+            }
+            /* Memory leak for every attached device if we don't
+             * test if the module is already loaded first */
             drv = xf86LookupInputDriver(option->value);
+            if (!drv)
+                if(xf86LoadOneModule(option->value, NULL))
+                    drv = xf86LookupInputDriver(option->value);
             if (!drv) {
                 xf86Msg(X_ERROR, "No input driver matching `%s'\n",
                         option->value);
-                return BadName;
+                rval = BadName;
+                goto unwind;
             }
             idev->driver = xstrdup(option->value);
             if (!idev->driver) {
-                xfree(idev);
-                return BadAlloc;
+                rval = BadAlloc;
+                goto unwind;
             }
         }
         if (strcmp(option->key, "name") == 0 ||
             strcmp(option->key, "identifier") == 0) {
+            if (idev->identifier) {
+                rval = BadRequest;
+                goto unwind;
+            }
             idev->identifier = xstrdup(option->value);
             if (!idev->identifier) {
-                xfree(idev);
-                return BadAlloc;
+                rval = BadAlloc;
+                goto unwind;
             }
         }
     }
+    if(!idev->driver || !idev->identifier) {
+        xf86Msg(X_ERROR, "No input driver/identifier specified (ignoring)\n");
+        rval = BadRequest;
+        goto unwind;
+    }
 
     if (!drv->PreInit) {
         xf86Msg(X_ERROR,
                 "Input driver `%s' has no PreInit function (ignoring)\n",
                 drv->driverName);
-        return BadImplementation;
+        rval = BadImplementation;
+        goto unwind;
     }
 
-    idev->commonOptions = NULL;
-    for (option = options; option; option = option->next)
+    for (option = options; option; option = option->next) {
+        /* Steal option key/value strings from the provided list.
+         * We need those strings, the InputOption list doesn't. */
         idev->commonOptions = xf86addNewOption(idev->commonOptions,
                                                option->key, option->value);
-    idev->extraOptions = NULL;
+        option->key = NULL;
+        option->value = NULL;
+    }
 
     pInfo = drv->PreInit(drv, idev, 0);
 
     if (!pInfo) {
         xf86Msg(X_ERROR, "PreInit returned NULL for \"%s\"\n", idev->identifier);
-        return BadMatch;
+        rval = BadMatch;
+        goto unwind;
     }
     else if (!(pInfo->flags & XI86_CONFIGURED)) {
         xf86Msg(X_ERROR, "PreInit failed for input device \"%s\"\n",
                 idev->identifier);
-        xf86DeleteInput(pInfo, 0);
-        return BadMatch;
+        rval = BadMatch;
+        goto unwind;
     }
 
     xf86ActivateDevice(pInfo);
 
     dev = pInfo->dev;
-    dev->inited = ((*dev->deviceProc)(dev, DEVICE_INIT) == Success);
+    ActivateDevice(dev);
     if (dev->inited && dev->startup)
         EnableDevice(dev);
 
     return Success;
+
+unwind:
+    if(pInfo) {
+        if(drv->UnInit)
+            drv->UnInit(drv, pInfo, 0);
+        else
+            xf86DeleteInput(pInfo, 0);
+    }
+    if(idev->driver)
+        xfree(idev->driver);
+    if(idev->identifier)
+        xfree(idev->identifier);
+    xf86optionListFree(idev->commonOptions);
+    xfree(idev);
+    return rval;
 }
 
 /*