input_server shouldn't use pointers as a cookie

[xyzzy]

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.

[korli]

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

[xyzzy]

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?

[korli]

Replying to xyzzy:

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

Sure, though a private protocol IMO

[bonefish]

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.

[xyzzy]

It does get used directly in one place, here: ​http://cgit.haiku-os.org/haiku/tree/src/servers/input/InputServer.cpp#n628

[leavengood]

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.

[leavengood]

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.

[korli]

Here is a proposed patch

[leavengood]

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.

[korli]

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