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

Avoid fatal errors on specific modules #401

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

Conversation

cwendling
Copy link
Member

No need to exit the whole MPM process if a module encounters a failure; otherwise the whole thing goes down.

Fixes #400.

⚠️ WARNING ⚠️ I didn't actually test any of this for the moment, because my setup is currently wonky for this; but it should be fairly straightforward anyhow (despite the fact that not erroring-out could have consequences, I don't think it's the case here given what I've seen).

@lukefromdc
Copy link
Member

lukefromdc commented Oct 2, 2024 via email

@cwendling
Copy link
Member Author

@lukefromdc First off, verifying it doesn't introduce any regression is a start :) Otherwise indeed I have no idea how to make the errors happen, short of modifying the code.

Maybe @mokraemer could try this PR and see if that helps?

@lukefromdc
Copy link
Member

lukefromdc commented Oct 2, 2024 via email

Copy link
Member

@lukefromdc lukefromdc left a comment

Choose a reason for hiding this comment

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

NOT tested for function as that cannot be done without a way to invoke the errors, but I got a clean build and looking at the code I see we are just removing g_error which calls abort() and replacing it with g_warning, g_critical (which does not exit by default but can be told to at runtime) and in one case removing a return statement after one of these since we are not aborting.

None of this should have any effect on mate-power-manager when running normally, all it does is remove instructions to abort on encountering an error. Only a test by someone who is getting these errors can determine if not aborting on them produces worse problems such as a potentially exploitable segfault.

@joakim-tjernlund
Copy link

I see on another users machine a bunch of:
Wed 2024-10-02 15:19:00 CEST 3568 125493 100 SIGTRAP present /usr/bin/mate-power-manager 919.2K

I would guess this patch prevents those, please merge it

@mokraemer
Copy link

Please check systemd version of the users machine. Patchlevel >=153-24 introduced a bug.

@joakim-tjernlund
Copy link

Please check systemd version of the users machine. Patchlevel >=153-24 introduced a bug.

systemd-256.6

@mokraemer
Copy link

I guess it has the same bug, as the patch is backported to the stable branch

@joakim-tjernlund
Copy link

I think mate-power-manager should not crash regardless of bug in systemd

@mokraemer
Copy link

true, but it will not work.
bug

As it can't connect to dbus

@joakim-tjernlund
Copy link

true, but it will not work. bug

As it can't connect to dbus

That is better than producing coredumps and filling up the disk.
This is an Desktop so does not matter that it won't work.

@mokraemer
Copy link

most desktop set core files to zero.
Just wanted to point out, the core of the problem lies in the fact the changed behaviour prevents any powermanger to work (e.g. kde is affected too).

@joakim-tjernlund
Copy link

So to be clear, this patch stopped mate-power-manager from crashing which was the goal.
systemd may be buggy but that is a different matter.
Please merge.

@mokraemer
Copy link

@joakim-tjernlund I am not fully on your side.
If some base component is broken, the application must and should stop working.
Producing core files is one of the only options to find such errors as you can load the core and see by the backtrace where the error happens - and this was the only way for me to find the problem.

If you patch out this behaviour, we propably would not have found it and still have the problem as no one wanted to debug the issue.

Most systems do not generate core files - they are disabled systemwide. So I suggest you do disable them in your system. Which leads to better results.

@joakim-tjernlund
Copy link

An app can choose to recover or exit gracefully if external input is faulty but not crash.

Crashing here is like your editor SEGV if you type the wrong cmd to it and that is not OK.

@mokraemer
Copy link

no that is quite different.
But it is not my part do decide this. I just wanted to reflect to you, that it might be desireable to have the software crash

In case of my editor, almost all of them crash, if e.g. the filesystem is inaccessable - which is compareable to the systemd failure.

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.

Crash on stale dbus file
4 participants