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

[firejail] Updated upstream to 0.9.72 Fixes JB#59121 #17

Merged
merged 1 commit into from Mar 22, 2023

Conversation

dsuni
Copy link

@dsuni dsuni commented Feb 17, 2023

No description provided.

rpm/firejail.spec Outdated Show resolved Hide resolved
@Thaodan
Copy link

Thaodan commented Feb 22, 2023 via email

Copy link
Member

@Tomin1 Tomin1 left a comment

Choose a reason for hiding this comment

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

Went through the privileged data patch and wrote some comments about the things I'd change. I think the simpler patches are good as they are. The remaining two big patches are such messy (as changes to patch files can be sometimes), that I'd need to have a separate look at them.

Copy link
Member

@Tomin1 Tomin1 left a comment

Choose a reason for hiding this comment

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

I think that INVALID_GID return value and adding that FIXME comment about those new groups would be something to do still. (The comment being something like // FIXME: Add missing debian specific groups, see JB#xxxxx.) I checked the rest of the patches and I think they are fine.

@Thaodan
Copy link

Thaodan commented Mar 16, 2023 via email

Copy link
Member

@Tomin1 Tomin1 left a comment

Choose a reason for hiding this comment

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

We don't use the Debian Groups, the other groups used here should be
replaced by systemd uaccess permissions.

I don't really care if the comment says, that is it about debian or that we should use some other mechanism, as long as it makes clear that we dropped code that is not relevant for us and we have it documented somewhere. So in short, some comment, some bug reference and I'm happy.

Something else strange that I noticed: This is built on top of commit 48a57d6 and not master (5b4cdb3). There is not real difference between those as it's just a merge commit that it's missing, so it can be just rebased there without changes. Also those new commits should be squashed of course.

@dsuni dsuni force-pushed the update_upstream_jb59121 branch 2 times, most recently from a8552a5 to 6704180 Compare March 17, 2023 10:47
@Tomin1
Copy link
Member

Tomin1 commented Mar 17, 2023

This is still on top of 48a57d6 and not master / 5b4cdb3 / 0.9.66+git3.

firejail (update_upstream_jb59121) [128]> git log --oneline
6704180 (HEAD -> update_upstream_jb59121, dsuni/update_upstream_jb59121) [firejail] Updated upstream to 0.9.72 Fixes JB#59121
48a57d6 (spiiroin/jb55650_libtrace_xstat, jb55650_libtrace_xstat) [libtrace] Add xstat() tracing and optionally log only failing calls. Fixes JB#55650
bedab8e (tag: 0.9.66+git2) Merge pull request #15 from spiiroin/jb55981_retain_symlink_chains

Otherwise I think it's good.

@dsuni
Copy link
Author

dsuni commented Mar 17, 2023

This is still on top of 48a57d6 and not master / 5b4cdb3 / 0.9.66+git3.

Fixed.

Copy link
Member

@Tomin1 Tomin1 left a comment

Choose a reason for hiding this comment

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

LGTM.

@Tomin1 Tomin1 merged commit 45dc642 into sailfishos:master Mar 22, 2023
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

5 participants