Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement memory for 16 rumble effects #191

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

implement memory for 16 rumble effects #191

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
47a8838
implement memory for 10 effects
Nov 5, 2021
2fc66dc
Increase limit to 16. Add bounds checking on upload/erase.
Nov 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions controller/controller.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,7 @@ void Controller::inputFeedbackReceived(
// Ignore other types of force feedback
if (effect.type != FF_RUMBLE)
{
Log::debug("Unknown effect %d", (int)effect.type);
return;
}

Expand Down
37 changes: 28 additions & 9 deletions controller/input.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,16 @@
#include <unistd.h>
#include <fcntl.h>

#define INPUT_MAX_FF_EFFECTS 1

InputDevice::InputDevice(
FeedbackReceived feedbackReceived
) : feedbackReceived(feedbackReceived)
{
for(int i=0;i<INPUT_MAX_FF_EFFECTS;i++)
{
effects[i] = {};
}


file = open("/dev/uinput", O_RDWR | O_NONBLOCK);

if (file < 0)
Expand Down Expand Up @@ -153,6 +157,8 @@ void InputDevice::handleFeedbackUpload(uint32_t id)

upload.request_id = id;

Log::debug("Got feedback upload %d", id);

if (ioctl(file, UI_BEGIN_FF_UPLOAD, &upload) < 0)
{
Log::error(
Expand All @@ -163,7 +169,7 @@ void InputDevice::handleFeedbackUpload(uint32_t id)
return;
}

effect = upload.effect;
effects[upload.effect.id] = upload.effect; //should really check the id first but the driver always seems to assign the first available from 0.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably should not be done without bounds checking... Especially since the array has a static size. You could look at kernel's ff-memless to look how many effect slots it supports. I believe it was 16. That way, both would behave the same.

But I also think that ff-memless combines effects with each other. With this implementation, only one effect at a time could play and would override the others.

upload.retval = 0;

if (ioctl(file, UI_END_FF_UPLOAD, &upload) < 0)
Expand All @@ -173,6 +179,8 @@ void InputDevice::handleFeedbackUpload(uint32_t id)
strerror(errno)
);
}

Log::debug("Uploaded effect id %d", upload.effect.id);
}

void InputDevice::handleFeedbackErase(uint32_t id)
Expand All @@ -181,6 +189,8 @@ void InputDevice::handleFeedbackErase(uint32_t id)

erase.request_id = id;

Log::debug("Got feedback erase %d", id);

if (ioctl(file, UI_BEGIN_FF_ERASE, &erase) < 0)
{
Log::error(
Expand All @@ -191,7 +201,7 @@ void InputDevice::handleFeedbackErase(uint32_t id)
return;
}

effect = {};
effects[erase.effect_id] = {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, bounds checking is missing. Maybe create accessor function instead of accessing the array directly, that is: make it private, create functions setEffect and getEffect.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using accessor function will make it easier later to change the implementation to maps if someone wants to. But I think a static array is perfectly okay, ff-memless does probably something similar: It also has a limited amount of memory. We can just skip the overhead from map lookups, a static array should be okay. Still using accessor function allows for limiting changes to code to one central place.

erase.retval = 0;

if (ioctl(file, UI_END_FF_ERASE, &erase) < 0)
Expand All @@ -201,15 +211,18 @@ void InputDevice::handleFeedbackErase(uint32_t id)
strerror(errno)
);
}

Log::debug("Erased effect id %d", erase.effect_id);
}

void InputDevice::handleEvent(input_event event)
{
Log::debug("input_event type %d code %d value %d", (int)event.type, (int)event.code, (int)event.value);
if (event.type == EV_UINPUT)
{
{//special event type, see uinput.h
if (event.code == UI_FF_UPLOAD)
{
handleFeedbackUpload(event.value);
handleFeedbackUpload(event.value); //value is the uinput event id, not to be confused with the request id
}

else if (event.code == UI_FF_ERASE)
Expand All @@ -223,12 +236,18 @@ void InputDevice::handleEvent(input_event event)
if (event.code == FF_GAIN)
{
// Gain varies between 0 and 0xffff
effectGain = event.value;
effectGain = event.value; //seems to be device-wide, not tied to any effect
Log::debug("Gain adjusted to %d", (int)effectGain);
}

else if (event.code == effect.id)
else if (event.code >= 0 && event.code < INPUT_MAX_FF_EFFECTS) //should really check if the effect has been erased or not
{
Log::debug("Triggering effect %d", (int)event.code);
feedbackReceived(effectGain, effects[event.code], event.value);
}
else
{
feedbackReceived(effectGain, effect, event.value);
Log::debug("Event code %d not handled", (int)event.code);
}
}
}
Expand Down
4 changes: 3 additions & 1 deletion controller/input.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@
#include <stdexcept>
#include <linux/uinput.h>

#define INPUT_MAX_FF_EFFECTS 10
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As already written, this should probably match with what ff-memless in the kernel supports.


/*
* User mode input device for gamepads
* Passes force feedback events to callback
Expand Down Expand Up @@ -91,7 +93,7 @@ class InputDevice
InterruptibleReader eventReader;
std::thread eventThread;

ff_effect effect = {};
ff_effect effects[INPUT_MAX_FF_EFFECTS] = {};
uint16_t effectGain = 0xffff;
FeedbackReceived feedbackReceived;
Copy link
Contributor

@kakra kakra Nov 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look, this is the problematic code: If you write out-of-bounds, you can overwrite the function pointer in feedbackReceived - thus provoking a jump to unknown code which the attacker could also conveniently pass through out-of-bounds writes.

};
Expand Down