evdev problems

[Daniel Stone]

On Wed, Mar 21, 2007 at 10:41:37PM +0100, Magnus Vigerlöf wrote:
> On Wednesday 21 March 2007 00:05, Daniel Stone wrote:
> > On Tue, Mar 20, 2007 at 11:53:09PM +0100, Magnus Vigerlöf wrote:
> > > I'm asking as I've been looking through the code for a few days to see
> > > what changes are needed in the wacom-driver to make it hotpluggable. So
> > > far I've seen memory-leaks, security holes, old limitations (and maybe
> > > some other, smaller things) in the server code, but not very much
> > > activity on the list addressing this. Maybe people are working, but I
> > > don't see it..
> >
> > Patches for leaks and pointing out limitations would be welcome; please
> > send xorg_security at x.org an email about the security holes.
> 
> Hmm.. I should have written 'hole'.. It has been pointed out in the 
> wiki/discussions here, so it's really not new; How can the path that are sent 
> into the server with the hotplug request (typically device-paths) be ensured 
> that they are really something allowed and should be readable by the Xserver 
> for the user?

They can't.  All we've really got to rely on here is the admin setting
the D-BUS security policy sanely.  One idea that I kicked around with
Peter and Zepheniah was to have the X server use HAL (although that
involves further use of libdbus, which we really want to avoid at all
costs), and get a list of input devices from HAL.  You offer the client
a selection of devices to use, and don't let them specify their own.

> The function configAddDevice [1] allocate a list of InputOption's that is 
> never freed. And in NewInputDeviceRequest [2] there will be a leak if there 
> are multiple device, name, or identifier options in the request. Not serious 
> ones, but I'll get to them in time.
> But the most interesting one I found was in the option list functions [3]. The 
> function addNewOption2 [4] can replace the values of an existing option in 
> the list, but it doesn't free the old values. But there are some values that 
> are not allocated on the heap that will be put into these lists as well, so 
> those places must be updated before the free is added here.
> 
> I think I'll start with the last one unless there are plans on replacing them 
> with some other structure..
> 
> What's best, to send the patches to the list or register a bug? Other options?

There are no real plans to change this, so if you send patches to the
list, I'll merge them.  (I know I have some others pending -- sorry,
been busy.)  That'd be great, thanks.

> I'm also concerned about which memory actually gets freed when the 
> remove-request arrives for a hot-plugged device. But I haven't got that far 
> yet as the wacom driver is no saint in that part either.

Yeah, it all really depends on the DDX and on the device.  As far as I
know, the DIX part doesn't have any leaks there.

> The 'old limits' part is the 20-device limit (inputInfo.numDevices) which is 
> increased for every call to xf86ActivateDevice [5,6] but never decreased. 
> When it get past 20 FatalError is called even if there's maybe only 4 devices 
> in the list... Would it be desireable to have an ever increasing counter or 
> is it possible to reuse the ids as soon as they are freed?

Oh god, I didn't even realise that.  We can bump the limit up by having
it dynamically allocated.

> > If you have any questions, please ask, and we'll fill you in to the best
> > of our ability.
> 
> Ok, first ones :)
> 
> There's a proposal on the wiki [7] which outlines the functions that could be 
> used for what purpose for input devices. Last edit is Sept '05, so one wonder 
> is this is a good reference to use when updating the current device drivers?

No, it was never implemented at all, and doesn't necessarily bear much
relation to what's been done.  To be honest, the input side of things
isn't really documented at all.

> Also, the 'PreInit' function [8] is currently mandatory, but the 'UnInit' 
> function is never even called (even if it's defined). In a static defined 
> config I guess it doesn't really matter, but I like having balanced 
> create/destroy calls.. So should we eliminate PreInit or start using UnInit 
> (if defined)?

Starting to use UnInit would be more sensible, IMO.

Thanks for your work!

Cheers,
Daniel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.x.org/archives/xorg/attachments/20070321/cb501d0e/attachment.pgp>