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

[Eric Blake]

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?

> +
> +    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.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org