Bug 24922

Summary:

xf86Wakeup clears invalid fd in fd_set

Product:

xorg

Reporter:

Chase Douglas <chasedouglas>

Component:

Server/General

Assignee:

Peter Hutterer <peter.hutterer>

Status:

RESOLVED FIXED

QA Contact:

Xorg Project Team <xorg-team>

Severity:

normal

Priority:

medium

Keywords:

patch

Version:

git

Hardware:

All

OS:

All

Whiteboard:

i915 platform:

i915 features:

Attachments:

Description	Flags
Potential patch fix	none
Git formatted potential patch fix	none

Description Chase Douglas 2009-11-04 11:17:45 UTC

All line numbers reference the current git master HEAD (7442f3355ab8f0bb2b1a270da18c65d8d315d4dd)

At line 272 of hw/xfree86/common/xf86Events.c, FD_CLR() is called using the fd from pInfo->fd. To get to this line of code, pInfo->fd must be sane (>= 0) in line 262. However, the evdev driver's read_input function, called in line 266 through pInfo->read_input(), sets pInfo->fd to -1 under some set of conditions. This causes the FD_CLR to crash Xorg.

Possible solutions:

1. Move the FD_CLR above the pInfo->read_input() call
2. Fix the evdev driver if it shouldn't be invalidating pInfo->fd

Here's the gist of my reproduction scenario: I'm the developer of launchpad.net/rinput. It's a server that listens for remote input event clients. The clients tell the server what kind of input device they are (keyboard, mouse, etc.), and the server creates a new Linux uinput device for the client. The client sends input events to the server, which sends them through uinput to the kernel and eventually back up to Xorg. I connect to the server up to about 10 times per day, and every few days the events stop propagating through Xorg. When I kill rinput Xorg crashes. I used one of the crash dumps to determine where the issue was.

Comment 1 Chase Douglas 2009-11-09 09:26:55 UTC

Created attachment 31070 [details] [review]
Potential patch fix

The attached patch merely moves the FD_CLR() call before the pInfo->read_input() call. I believe this fixes the issue, and I am testing it on my own system right now.

Comment 2 Peter Hutterer 2009-11-09 19:38:34 UTC

Please attach this patch as a signed-off git-formatted patch. For more info, see
http://wiki.x.org/wiki/Development/Documentation/SubmittingPatches.

Then attach the patch here. I'll take it into my tree then.

Comment 3 Chase Douglas 2009-11-09 19:58:16 UTC

Created attachment 31076 [details] [review]
Git formatted potential patch fix

Comment 4 Peter Hutterer 2009-11-09 20:40:54 UTC

Thanks. On second look at it, I think the move of the FD_CLR is correct, but
unblocking the SIGIO must happen after ReadInput.
ReadInput causes the events to be passed to the server and - amongst other
things - the pointer to move. The whole reason for using SIGIO here is to
get more responsive cursor movement, so we can't unblock until that's
complete.

Comment 5 Chase Douglas 2009-11-10 15:50:59 UTC

(In reply to comment #4)
> Thanks. On second look at it, I think the move of the FD_CLR is correct, but
> unblocking the SIGIO must happen after ReadInput.
> ReadInput causes the events to be passed to the server and - amongst other
> things - the pointer to move. The whole reason for using SIGIO here is to
> get more responsive cursor movement, so we can't unblock until that's
> complete.

Maybe I don't quite understand what you are saying, but I think the patch is in line with what you've stated. You seem to be saying the the SIGIO unblocking must follow the call to pInfo->read_input. That sequence of events is still preserved in the patch.

If I misinterpreted your statement, could you try restating it for me?

Thanks

Comment 6 Peter Hutterer 2009-11-10 15:56:31 UTC

I need to either drink more coffee or less, not sure which one. sorry about that, ignore my previous comment.

This patch will be included in my next pull request to master, I'll close the bug once it is upstream.

Comment 7 Peter Hutterer 2009-11-10 20:25:50 UTC

Pushed to master as b5aa2e0a5fe233dc883084a5026013472e85bca4. Thanks again for the patch!

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.