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

Do not hard-fail on process owner check #177

Merged
merged 3 commits into from
Nov 6, 2022

Conversation

zopieux
Copy link
Contributor

@zopieux zopieux commented Nov 5, 2022

ps might be missing in $PATH, or fan2go might be running as a non-root user with write access to the subsystem. I don't think it's reasonable to crash the program for a simple uid check that might actually be wrong. This will eventually crash later when trying to modify paths as non-root anyway.

`ps` might be missing in $PATH, or fan2go might be running as a non-root user with write access to the subsystem. I don't think it's reasonable to crash the program for a simple uid check that might actually be wrong. This will eventually crash later when trying to modify paths as non-root anyway.
@markusressel
Copy link
Owner

Hey, thx for opening the PR! ❤️

There are definitely scenarios in which you do not need to use root permissions for running fan2go, so I can absolutely see why we need to improve the current logic 👍

ps might be missing in $PATH, or fan2go might be running as a non-root user with write access to the subsystem. I don't think it's reasonable to crash the program for a simple uid check that might actually be wrong.

I agree, this code is a remnant from the early days and also my specific usage. However, I think we need to be a bit more specific here, since there is an additional permission related check done while evaluating the configuration file meant to protect users against malicious configuration editors. Did you consider this?

This will eventually crash later when trying to modify paths as non-root anyway.

Did you validate this? Because I am not 100% sure what the current behavior would be 🤔
When a fan cannot be controlled during normal operation, the error should not lead to a crash, but it might do that when the error is already present during startup (of the fan2go service).

@zopieux
Copy link
Contributor Author

zopieux commented Nov 6, 2022

Did you validate this?

I did not, "crash" might be the wrong word here. I meant that I/O while trying to set the PWM will fail in a way or another and hopefully bubble up in the logs, so it'll be pretty obvious something's wrong.

Did you consider this?

I'm not sure what malicious editor protection you're referring to here, sorry, I only skimmed at the surface of fan2go – thanks btw, what a pleasure it was jettisoning that fancontrol garbage!

I don't really mind how this check is done, I just believe depending on a subprocess invocation & a (very common but not necessarily available) Linux util is overkill for a simple user warning message. Maybe Go has a way to report its own uid, like this?

Feel free to close this PR and come up with something better :)

@markusressel
Copy link
Owner

I don't really mind how this check is done, I just believe depending on a subprocess invocation & a (very common but not necessarily available) Linux util is overkill for a simple user warning message. Maybe Go has a way to report its own uid, like this?

Good find! Seems like a much more reasonable approach 👍

Feel free to close this PR and come up with something better :)

I think we can use it for now, I will think about other special cases in a later PR.

@markusressel markusressel merged commit ae82dec into markusressel:master Nov 6, 2022
@zopieux zopieux deleted the nonfatal-owner-check branch November 6, 2022 19:54
ui.Warning("Unable to verify process owner: %v", err)
}
if owner != "root" {
ui.Info("fan2go is running as a non-root user '%s'. If you encounter errors, make sure to give this user the required permissions.", owner)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice! Making it non-fatal makes a lot of sense actually.

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