search

USBGuard / usbguard

USBGuard is a software framework for implementing USB device authorization policies (what kind of USB devices are authorized) as well as method of use policies (how a USB device may interact with the system)
https://usbguard.github.io/
GNU General Public License v2.0
1.1k stars 133 forks source link

Unreference PolkitAuthorizationResult and PolkitAuthority structs if needed #551

Closed Cropi closed 1 year ago

Cropi commented 1 year ago
  1. If polkit_authority_get_sync() fails, then it will return null, so there is no need to free it with g_object_unref().
  2. The same goes for polkit_authority_check_authorization_sync(). The documentation says the following: "A PolkitAuthorizationResult is returned or NULL if error is set. Free with g_object_unref()."

I came across this issue when SElinux did not allow usbguard-dbus to perform the mentioned actions. Thus, usbguard tried to free memory, which was not even allocated.

[1] https://www.freedesktop.org/software/polkit/docs/latest/PolkitAuthority.html

hartwork commented 1 year ago

@Cropi PS: For the CI failure with Ubuntu 21.10, I believe the main point was to cover the most recent Ubuntu, more recent than GitHub CI's ubuntu-latest, so maybe we should migrate to Ubuntu 22.04 there, ideally in a dedicated pull request. I have prepared pull request #552 for that now and we could rebase #551 on top of that, after #552 is merged, if you like. What do you think?

Cropi commented 1 year ago

Hi @Cropi ,

for review of this pull request I asked myself two questions:

* Can function `polkit_authority_get_sync` return `NULL`?
  I found the answer in file `polkit-v.121/src/polkit/polkitauthority.c`: Yes, it can.

* Can function `g_object_unref` be called with `NULL`, safely?
  After some digging, the answer is: No, it cannot.

With those two answers I agree that this pull request is moving the code into the right direction and fixes a real problem +1 . I would suggest to also drop the second || error from ! authority || error for both consistency and readability, and then I'd consider it "perfect". What do you think?

Best, Sebastian

Thanks for the review. I definitely agree that we should change the authority part as well to ensure better consistency.

hartwork commented 1 year ago

@Cropi with PRs #548, #551, #552 all merged now — how do you feel about cutting a release 1.1.2 off master?

Cropi commented 1 year ago

@Cropi with PRs #548, #551, #552 all merged now — how do you feel about cutting a release 1.1.2 off master?

I am not against it but usually @radosroka is the one who creates the releases.

hartwork commented 1 year ago

@radosroka any chance or objections?

radosroka commented 1 year ago

@radosroka any chance or objections?

I can create release later today.

hartwork commented 1 year ago

@radosroka cool! Thank you!

radosroka commented 1 year ago

@radosroka cool! Thank you!

Release is out there:

https://github.com/USBGuard/usbguard/releases/tag/usbguard-1.1.2

hartwork commented 1 year ago

@radosroka thank you! Bumped to 1.1.2 in Gentoo as well now.