On Mon, Oct 24, 2011 at 16:48:58 -0600, Eric Blake wrote: > On 10/19/2011 11:26 AM, Jiri Denemark wrote: > > When saving config files we just overwrite old content of the file. In > > case something fails during that process (e.g. disk gets full) we lose > > both old and new content. This patch makes the process more robust by > > writing the new content into a separate file and only if that succeeds > > the original file is atomically replaced with the new one. > > --- > > src/libvirt_private.syms | 1 + > > src/util/virfile.c | 56 > > ++++++++++++++++++++++++++++++++++++++++++++++ > > src/util/virfile.h | 6 +++++ > > 3 files changed, 63 insertions(+), 0 deletions(-) > > > +virFileRewrite(const char *path, > > + mode_t mode, > > + virFileRewriteFunc rewrite, > > + void *opaque) > > +{ > > + char *newfile = NULL; > > + int fd = -1; > > + int ret = -1; > > + > > + if (virAsprintf(&newfile, "%s.new", path)< 0) { > > + virReportOOMError(); > > + goto cleanup; > > + } > > + > > + if ((fd = open(newfile, O_WRONLY | O_CREAT | O_TRUNC, mode))< 0) { > > Should we be using O_EXCL instead of O_TRUNC, and insisting that no .new > file already exists, so as to protect ourselves from symlink attacks > where someone installs a symlink and tricks us into truncating a random > file?
I think this would need to be configurable and can be safely deferred to the future when there is a need to use this API for other purposes than writing libvirt's XML files. In this case we don't care about existing .new files and it's much easier for the user running libvirtd to replace a random file than trick libvirtd to do it. > > + > > + if (rename(newfile, path)< 0) { > > + virReportSystemError(errno, _("cannot rename file '%s' as '%s'"), > > + newfile, path); > > + goto cleanup; > > + } > > + > > + ret = 0; > > + > > +cleanup: > > + VIR_FORCE_CLOSE(fd); > > + if (newfile) { > > + unlink(newfile); > > This fails if the rename() succeeds, or worse, it succeeds at unlinking > an unrelated file that someone created in the window between our rename > and this unlink. We should probably only attempt the unlink if ret < 0. OK, although someone else could have unlinked and created the same file between our open and unlink, too :-) Note that none of these situation can happen since we only use this function to save libvirt's XML files and only one thread can create a file with the same name. Jirka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list