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

Fix file ownership after pihole-FTL --config as root #2145

Merged
merged 3 commits into from
Jan 6, 2025

Conversation

catch-error
Copy link
Contributor

What does this PR aim to accomplish?:

When calling pihole-FTL --config key value as root, the resulting pihole.toml and dnsmasq.conf files in /etc/pihole are owned by root:root. It should be pihole:pihole.

How does this PR accomplish the above?:

pihole.toml: Root cause is that when openFTLtoml() is called with mode "w", it'll not open pihole.toml, but pihole.toml.tmp instead. closeFTLtoml(), however, always calls chown_pihole() on pihole.toml. As in the next step pihole.toml.tmp gets renamed to pihole.toml, its root:root ownership persists.

This patch evaluates the open mode in closeFTLtoml(), changing the ownership of pihole.toml.tmp if the mode isn't O_RDONLY.

dnsmasq.conf: Add a call to chown_pihole() for the temporary file, if called as root.

Link documentation PRs if any are needed to support this PR:

N/A


By submitting this pull request, I confirm the following:

  1. I have read and understood the contributors guide, as well as this entire template. I understand which branch to base my commits and Pull Requests against.
  2. I have commented my proposed changes within the code and I have tested my changes.
  3. I am willing to help maintain this change if there are issues with it later.
  4. It is compatible with the EUPL 1.2 license
  5. I have squashed any insignificant commits. (git rebase)
  6. I have checked that another pull request for this purpose does not exist.
  7. I have considered, and confirmed that this submission will be valuable to others.
  8. I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  9. I give this submission freely, and claim no ownership to its content.

  • I have read the above and my PR is ready for review. Check this box to confirm

Copy link
Member

@DL6ER DL6ER left a comment

Choose a reason for hiding this comment

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

Thanks for your PR! I have only very litte suggestions. The const are really just to try to prevent possible new mistakes in the future. The larger suggestion is to document the logic in the code. Any compiler will wipe out the const bool during optimization so this should be equivalent in actual machine code to your initial suggestion but IMO is much more readable for humans.

src/config/toml_helper.c Outdated Show resolved Hide resolved
src/config/toml_helper.c Outdated Show resolved Hide resolved
src/config/toml_helper.c Outdated Show resolved Hide resolved
@catch-error
Copy link
Contributor Author

Thx for your suggestions @DL6ER. They're done now.

@DL6ER
Copy link
Member

DL6ER commented Dec 30, 2024

We're currently seeing issues with the CI, seems ARM runner are currently unavailable. I'll approve and merge once this is resolved

@DL6ER
Copy link
Member

DL6ER commented Dec 30, 2024

Actually,

Screenshot_2024-12-30-17-06-17-55_3aea4af51f236e4932235fdada7d1643

@catch-error
Copy link
Contributor Author

Strange... I've now run builds on Raspi OS 12 (aarch64), Debian 12 (x86_64), Ubuntu 24.04 (aarch64 & x86_64) and Fedora 40 (aarch64 & x86_64). All builds go through w/o issues.

toml_helper.c includes sys/file.h, which in turn includes fcntl.h. I can add an explicit include to fcntl.h as well, looks like some environments need this.

@DL6ER
Copy link
Member

DL6ER commented Dec 30, 2024

The CI builds against libmusl on Alpine Linux - from what I can see, all your test systems use GNU libc. Alpine Linux ships with the rather new gcc v14.2.0.

@catch-error
Copy link
Contributor Author

Ok, I see. I need to enhance my build environments. It now also compiles on Alpine Linux.

@catch-error catch-error requested a review from DL6ER January 6, 2025 06:04
@DL6ER DL6ER merged commit e73c4e2 into pi-hole:development Jan 6, 2025
13 checks passed
@catch-error catch-error deleted the fix/chown-pihole branch January 12, 2025 10:53
@PromoFaux PromoFaux mentioned this pull request Feb 18, 2025
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.

2 participants