Skip to content

Ensure lxc config is saved atomically#4562

Open
emirbuljubasic wants to merge 2 commits into
lxc:mainfrom
emirbuljubasic:fix/api-config-save
Open

Ensure lxc config is saved atomically#4562
emirbuljubasic wants to merge 2 commits into
lxc:mainfrom
emirbuljubasic:fix/api-config-save

Conversation

@emirbuljubasic
Copy link
Copy Markdown

This PR fixes an issue where save_config could leave the container config file empty if a write fails after truncation. The current implementation opens the config file with O_TRUNC, which clears its contents before writing. In cases of filesystem issues or write errors, this results in an empty config.

This change writes the updated configuration to a temporary file first, and then performs an atomic rename() to replace the original config file. This ensures that the config file is only replaced if the write succeeds, preventing accidental data loss.

Fixes #4561

Signed-off-by: emirbuljubasic <emirbuljubasic329@gmail.com>
@stgraber
Copy link
Copy Markdown
Member

stgraber commented Jul 3, 2025

Hmm, what happens if I have custom ownership (user/group), ACLs or xattrs on the config file?

@mihalicyn mihalicyn self-requested a review July 25, 2025 15:23
DreamConnected added a commit to Container-On-Android/lxc that referenced this pull request Jul 30, 2025
Added support for restoring file ownership, ACLs, and attributes.

Signed-off-by: DreamConnected <1487442471@qq.com>
Co-Authored-By: Emir Buljubašić <34354939+emirbuljubasic@users.noreply.github.com>
DreamConnected added a commit to Container-On-Android/lxc that referenced this pull request Aug 1, 2025
Added support for restoring file ownership, ACLs, and attrs.

Signed-off-by: DreamConnected <1487442471@qq.com>
Co-Authored-By: Emir Buljubašić <34354939+emirbuljubasic@users.noreply.github.com>
Comment thread src/lxc/lxccontainer.c
S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP);
if (fd_config < 0) {
SYSERROR("Failed to open config file \"%s\"", alt_file);
char tmpfile[PATH_MAX];
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We don't really know where/how lxc was compiled, so PATH_MAX could be really short. Worth adding an assert that it will be big enough for the tempfile path.

Comment thread src/lxc/lxccontainer.c
close(tmpfd);
tmpfd = -EBADF;

if (chmod(tmpfile, S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP) < 0) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As stgraber pointed out, I think you need to stat the original file and make sure to re-set the owner, group, perms, and acls.

Have you seen the condition you are worried about (failure between open(truncate) and write/flush) happen?

Would it be easier to just create a backup of the file first, and write to the original? (I'm not sure)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I completely forgot I made this PR. Thank you and stgraber for the inputs, much appreciated!

Yes this issue has happened to me on multiple ocassions, although quite hard to reproduce. I did add some logs to this part of the API and was able to confirm that this was where the issue was coming from, file getting truncated and write failing for one reason or another.

I will try to update this PR ASAP, hopefully in the next few days at the latest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

LXC config file empty after calling save_config API

3 participants