Repository navigation
daemon: Use systemd to validate xdg-desktop-portal process - #242
Conversation
335b3b5 to
5e5b009
Compare
msirringhaus
left a comment
msirringhaus
left a comment
There was a problem hiding this comment.
Overall looking good. The new Pid-class looks overly complicated, unless I missed something.
|
|
||
| struct Pid<'a> { | ||
| raw_pid: u32, | ||
| _marker: PhantomData<&'a ()>, |
There was a problem hiding this comment.
Is this really needed? Looks like we only use it in one function anyways, or did I miss anything? Can't this just be a raw u32?
There was a problem hiding this comment.
I have a plan to use this in other places (communicating with credential providers). I wanted to communicate that it's only safe to use for the lifetime of the pidfd that was used to retrieve it. But maybe we can just make that implicit or a comment instead of abstracting too soon.
There was a problem hiding this comment.
Yeah, I think it's usually better to add complexity only when it's really needed. If some plans don't materialize, we are left with unnecessarily complicated code.
And if it should be tied to the pidfd, maybe it would also make sense to simply give it a reference to the relevant pidfd, instead of a phantom marker? And maybe even tie the functions that use the pidfd to it?
|
|
||
| impl Pid<'_> { | ||
| fn value(&self) -> &u32 { | ||
| &self.raw_pid |
There was a problem hiding this comment.
Every usage of this function immediately dereferences it. Can't we just return a plain u32 here?
There was a problem hiding this comment.
Probably. Do you think the reference on the Pid<'_> itself probably communicates the lifetime well enough?
I also realize that I don't need to derefence the other places, since we're just using it in format strings, where reference and u32 is represented the same way.
There was a problem hiding this comment.
As mentioned above: Depending on the future plans, it might be simpler to tie should_trust_app_id() to the new Pid-struct, which should probably not be called Pid then, but AppId or something. This could then have various functions for verifying the AppId, and internally query the Pid as well (tied to the lifetime of the pidfd, which it also owns).
Co-Authored-By: Marie Ramlow <me@nycode.dev>
5e5b009 to
0597756
Compare
|
Good points on the |
2 participants
Adds systemd as another source of validation. Still falls back to path-based checks if systemd check fails.
Alternative to #199.