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.

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

liboxide: Catch stoi() exceptions #302

Merged
merged 3 commits into from
May 10, 2023

Conversation

alistair23
Copy link
Contributor

Avoid crashing tarnish if the string provided to SysObject isn't a valid string.

Avoid crashing tarnish if the string provided to SysObject isn't a valid
string.

Signed-off-by: Alistair Francis <alistair@alistair23.me>
@Eeems
Copy link
Collaborator

Eeems commented May 9, 2023

Do you have a test case for this?
From what I'm seeing I think I'd prefer to fix tarnish to not pass in invalid strings to this. Assuming we know where that is happening.

@alistair23
Copy link
Contributor Author

The test case is a buggy max77818 driver that doesn't supply /sys/class/power_supply/max77818_battery/present to userspace. It isn't really something we need to handle nicely, but it would be great not to crash

@Eeems
Copy link
Collaborator

Eeems commented May 9, 2023

The test case is a buggy max77818 driver that doesn't supply /sys/class/power_supply/max77818_battery/present to userspace. It isn't really something we need to handle nicely, but it would be great not to crash

That should still be caught and return a 0 with a warning logged out by strProperty, no? Or is it trying to read it and getting a non integer value?

I'm not seeing crashes logged for this, and I would expect to see quite a few, as well as reports that oxide isn't working for people.

@alistair23
Copy link
Contributor Author

That should still be caught and return a 0 with a warning logged out by strProperty, no? Or is it trying to read it and getting a non integer value?

It's trying to read it and getting a non-integer value, which then triggers the stoi() exception

I'm not seeing crashes logged for this, and I would expect to see quite a few, as well as reports that oxide isn't working for people.

I noticed this while re-writing the max77818 driver in preparation for the 6.3 kernel release and eventual upstreaming, so I don't think anyone would be reporting crashes

@Eeems
Copy link
Collaborator

Eeems commented May 10, 2023

Okay, that makes sense now. I'll get this merged in a bit. Just trying to recover my reMarkable after bricking it when testing a change to the kernelctl package 😛

@Eeems Eeems merged commit b27e56c into Eeems-Org:master May 10, 2023
@alistair23 alistair23 deleted the alistair/battery-crash-fix branch May 10, 2023 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants