search

evilsocket / opensnitch

OpenSnitch is a GNU/Linux interactive application firewall inspired by Little Snitch.
GNU General Public License v3.0
9.86k stars 486 forks source link

Fix SIGSEGV caused by uninitialized firewall and logger #1130

Closed 1fishe2fishe closed 1 month ago

1fishe2fishe commented 1 month ago

I compile opensnitch from git, and I got a journalctl backlog full of these today: image This seemed to be related to bc3209494524497566a72a973f5b9ce1df36e67f, and after restoring firewall.Init(), I got this: image This change to daemon/ui/config_utils.go, where SetLoggers() is called, changes it to only call it on one branch, so that seemed to be where the null logger problem was introduced.

My change seems to work properly without issues, but I'm not a Go person so feel free to correct me.

gustavo-iniguez-goya commented 1 month ago

hey @1fishe2fishe , thank you for taking a look at this!

Could you verify that latest sources compile fine? I added more changes after this commit, sorry for that O:)

1fishe2fishe commented 1 month ago

Well, it does compile fine with the latest commit, but the problem still lingers. I think the fix is still needed, and my patch doesn't have conflicts with the master branch (at least as of c9ad9005e346b5e2bcf21311b2db8dec47985890), so I guess it's good to merge. image

1fishe2fishe commented 1 month ago

That is to say, with my patch it runs just fine and it successfully works and connects to the UI without segfaulting or anything. image

gustavo-iniguez-goya commented 1 month ago

no problem with the patch, it actually fixes the crashes, but let's fix it more in line with the latest changes (avoiding config reloading as much as possible).

Could you try the following changes and let me know if they fixes the crashes on your machine?

add stats.SetLoggers(loggerMgr) after line 594: https://github.com/evilsocket/opensnitch/blob/c9ad9005e346b5e2bcf21311b2db8dec47985890/daemon/main.go#L593-L595

and remove "reload" here: https://github.com/evilsocket/opensnitch/blob/c9ad9005e346b5e2bcf21311b2db8dec47985890/daemon/ui/config_utils.go#L168

1fishe2fishe commented 1 month ago

That compiled and ran without issues, thanks for fixing the problem. I don't have a good understanding of this codebase/language, so I'll close the PR and issue in the meantime and wait for you to push the more efficient fix instead of what is essentially a commit revert.

gustavo-iniguez-goya commented 1 month ago

Thank you @1fishe2fishe for taking a look at this and report it! These changes have been waiting for a long time, and there will be more, so more testing and unit tests will be needed.