Skip to content

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.

2 participants