Opened 7 years ago

Closed 7 years ago

#8831 closed bug (fixed)

input_server shouldn't use pointers as a cookie

Reported by: xyzzy Owned by: korli
Priority: normal Milestone: R1
Component: Servers/input_server Version: R1/Development
Keywords: Cc:
Blocked By: Blocking:
Has a Patch: yes Platform: All

Description

The input_server currently uses pointers as a cookie in its communication with the input method replicant to identify an input method, and stores them in an int32 field in BMessage. This is problematic on x86_64 due to the increased pointer width. As a temporary workaround I have changed the cookie field in the messages to be a pointer rather than an int32 in my branch. However, pointers should not be used in cross-app communication, an integer ID should be used instead.

Attachments (1)

0001-input_server-use-a-generated-cookie-instead-of-point.patch (5.3 KB ) - added by korli 7 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 by korli, 7 years ago

I don't know whether this case is really about cross-app communication because code for replicant and server is the same binary.

in reply to:  1 ; comment:2 by xyzzy, 7 years ago

Replying to korli:

I don't know whether this case is really about cross-app communication because code for replicant and server is the same binary.

Doesn't the replicant code run in the deskbar, though?

in reply to:  2 comment:3 by korli, 7 years ago

Replying to xyzzy:

Doesn't the replicant code run in the deskbar, though?

Sure, though a private protocol IMO

comment:4 by bonefish, 7 years ago

I haven't really looked what happens with the pointer, particularly whether it is sent back to the input server and how it is used there. E.g. dangerous code in the input server would be:

void* cookie;
if (message.FindPointer("cookie", &cookie) == B_OK) {
	BInputServerMethod* method = (BInputServerMethod*)cookie;
	method->DoSomething();
}

This way a bug in the application hosting the replicant (i.e. the Deskbar) could compromise the input server. Harmless code would be:

void* cookie;
if (message.FindPointer("cookie", &cookie) == B_OK) {
	foreach (BInputServerMethod* method = ...) {
		if (cookie == method)
			method->DoSomething();
	}
}

In this case the only bad thing that could happen is that the wrong method is targeted due to pointer reuse. Though I guess that would be rather unlikely and harmless as well.

comment:6 by leavengood, 7 years ago

I don't see any real reason why it can't be just an int ID, beyond the time needed to make the change and test it.

And given that Alex has shown it does use the pointer in a dangerous way, we probably should change it.

If the input methods and ID generation would be all internal to the input_server, a static method would do the job to get unique IDs.

Maybe a hash_map could be used as well to speed up looking for input methods based on the cookie.

comment:7 by leavengood, 7 years ago

Looking more at the input_server code it already maintains a list of input methods, so line 628 could be changed to look for the input method based on the cookie (by comparing the pointer), removing the dangerous pointer usage. Then the cookie could be changed to an ID without too much trouble I think. But that would not be as big of a priority once the dangerous pointer usage is removed.

There is plenty of padding in BInputServerMethod to add a cookie, which as I suggested could be generated in a static method of BInputServerMethod.

comment:8 by korli, 7 years ago

Has a Patch: set

comment:9 by korli, 7 years ago

Here is a proposed patch

comment:10 by leavengood, 7 years ago

That is pretty much what I was thinking, except maybe putting the generation of the next cookie in a static function just to make it clear. Though this is probably OK.

comment:11 by korli, 7 years ago

Resolution: fixed
Status: newclosed

Fixed in hrev44589. I had to check for gKeymapMethod though, as it's not in the input methods list.

Note: See TracTickets for help on using tickets.