[libvirt] [PATCH 2/4] Introduce virFileRewrite for safe file rewrite

[Jiri Denemark]

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