[PATCH] ldconfig: Call fsync on temporary files before renaming them [BZ #20890]

[PATCH] ldconfig: Call fsync on temporary files before renaming them [BZ #20890]

[Florian Weimer]

If the system crashes before the file data has been written to disk, the
file system recovery upon the next mount may restore a partially
rewritten temporary file under the non-temporary (final) name (after the
rename operation).

Some file systems perform an implicit fsync before renaming a file over
another one, but XFS does not, for example.

2018-01-12  Florian Weimer  <fweimer@redhat.com>

	[BZ #20890]
	* elf/cache.c (save_cache): Call fsync on temporary file before
	renaming it.
	(save_aux_cache): Likewise.

diff --git a/elf/cache.c b/elf/cache.c
index 1ec6ab36e7..2165f8d305 100644
--- a/elf/cache.c
+++ b/elf/cache.c
@@ -449,7 +449,8 @@ save_cache (const char *cache_name)
     }
 
   if (write (fd, strings, total_strlen) != (ssize_t) total_strlen
-      || close (fd))
+      || fsync (fd) != 0
+      || close (fd) != 0)
     error (EXIT_FAILURE, errno, _("Writing of cache data failed"));
 
   /* Make sure user can always read cache file */
@@ -812,7 +813,8 @@ save_aux_cache (const char *aux_cache_name)
 
   if (write (fd, file_entries, file_entries_size + total_strlen)
       != (ssize_t) (file_entries_size + total_strlen)
-      || close (fd))
+      || fsync (fd) != 0
+      || close (fd) != 0)
     {
       unlink (temp_name);
       goto out_fail;

Re: [PATCH] ldconfig: Call fsync on temporary files before renaming them [BZ #20890]

[Christoph Hellwig]

On Fri, Jan 12, 2018 at 12:07:57PM +0100, Florian Weimer wrote:
> If the system crashes before the file data has been written to disk, the
> file system recovery upon the next mount may restore a partially
> rewritten temporary file under the non-temporary (final) name (after the
> rename operation).
> 
> Some file systems perform an implicit fsync before renaming a file over
> another one, but XFS does not, for example.

At least on Linux no file system performs an actual fsync equivalent.
A few do start a writeout, but don't actually wait on it.

Re: [PATCH] ldconfig: Call fsync on temporary files before renaming them [BZ #20890]

[Florian Weimer]

On 01/12/2018 12:37 PM, Christoph Hellwig wrote:
> On Fri, Jan 12, 2018 at 12:07:57PM +0100, Florian Weimer wrote:
>> If the system crashes before the file data has been written to disk, the
>> file system recovery upon the next mount may restore a partially
>> rewritten temporary file under the non-temporary (final) name (after the
>> rename operation).
>>
>> Some file systems perform an implicit fsync before renaming a file over
>> another one, but XFS does not, for example.
> 
> At least on Linux no file system performs an actual fsync equivalent.
> A few do start a writeout, but don't actually wait on it.

Oh.  I'll drop the last paragraph from the commit message, then.

Thanks,
Florian

Re: [PATCH] ldconfig: Call fsync on temporary files before renaming them [BZ #20890]

[Paul Eggert]

Florian Weimer wrote:
>     if (write (fd, strings, total_strlen) != (ssize_t) total_strlen
> -      || close (fd))
> +      || fsync (fd) != 0
> +      || close (fd) != 0)
>       error (EXIT_FAILURE, errno, _("Writing of cache data failed"));

Would fdatasync suffice, instead of fsync?

Re: [PATCH] ldconfig: Call fsync on temporary files before renaming them [BZ #20890]

[Florian Weimer]

[-- Attachment #1: Type: text/plain, Size: 546 bytes --]

On 01/13/2018 03:28 AM, Paul Eggert wrote:
> Florian Weimer wrote:
>>     if (write (fd, strings, total_strlen) != (ssize_t) total_strlen
>> -      || close (fd))
>> +      || fsync (fd) != 0
>> +      || close (fd) != 0)
>>       error (EXIT_FAILURE, errno, _("Writing of cache data failed"));
> 
> Would fdatasync suffice, instead of fsync?

One can be fdatasync, I think.

The first one should actually happen after the chmod call, and then we 
need fsync.

Thanks,
Florian

[-- Attachment #2: ldconfig-fsync.patch --]
[-- Type: text/x-patch, Size: 1902 bytes --]

Subject: [PATCH] ldconfig: Sync temporary files to disk before renaming them [BZ #20890]
To: libc-alpha@sourceware.org

If the system crashes before the file data has been written to disk, the
file system recovery upon the next mount may restore a partially
rewritten temporary file under the non-temporary (final) name (after the
rename operation).

2018-01-12  Florian Weimer  <fweimer@redhat.com>

	[BZ #20890]
	* elf/cache.c (save_cache): Call fsync on temporary file before
	renaming it.
	(save_aux_cache): Call fdatasync on temporary file before renaming
	it.

diff --git a/elf/cache.c b/elf/cache.c
index 1ec6ab36e7..f032081e5c 100644
--- a/elf/cache.c
+++ b/elf/cache.c
@@ -448,8 +448,7 @@ save_cache (const char *cache_name)
 	error (EXIT_FAILURE, errno, _("Writing of cache data failed"));
     }
 
-  if (write (fd, strings, total_strlen) != (ssize_t) total_strlen
-      || close (fd))
+  if (write (fd, strings, total_strlen) != (ssize_t) total_strlen)
     error (EXIT_FAILURE, errno, _("Writing of cache data failed"));
 
   /* Make sure user can always read cache file */
@@ -458,6 +457,10 @@ save_cache (const char *cache_name)
 	   _("Changing access rights of %s to %#o failed"), temp_name,
 	   S_IROTH|S_IRGRP|S_IRUSR|S_IWUSR);
 
+  /* Make sure that data is written to disk.  */
+  if (fsync (fd) != 0 || close (fd) != 0)
+    error (EXIT_FAILURE, errno, _("Writing of cache data failed"));
+
   /* Move temporary to its final location.  */
   if (rename (temp_name, cache_name))
     error (EXIT_FAILURE, errno, _("Renaming of %s to %s failed"), temp_name,
@@ -812,7 +815,8 @@ save_aux_cache (const char *aux_cache_name)
 
   if (write (fd, file_entries, file_entries_size + total_strlen)
       != (ssize_t) (file_entries_size + total_strlen)
-      || close (fd))
+      || fdatasync (fd) != 0
+      || close (fd) != 0)
     {
       unlink (temp_name);
       goto out_fail;

Re: [PATCH] ldconfig: Call fsync on temporary files before renaming them [BZ #20890]

[Florian Weimer]

On 01/19/2018 05:39 PM, Florian Weimer wrote:
> 2018-01-12  Florian Weimer<fweimer@redhat.com>
> 
> 	[BZ #20890]
> 	* elf/cache.c (save_cache): Call fsync on temporary file before
> 	renaming it.
> 	(save_aux_cache): Call fdatasync on temporary file before renaming
> 	it.

Ping?

<https://sourceware.org/ml/libc-alpha/2018-01/msg00660.html>

Thanks,
Florian

Re: [PATCH] ldconfig: Call fsync on temporary files before renaming them [BZ #20890]

[Adhemerval Zanella]

On 20/02/2018 10:59, Florian Weimer wrote:
> On 01/19/2018 05:39 PM, Florian Weimer wrote:
>> 2018-01-12  Florian Weimer<fweimer@redhat.com>
>>
>>     [BZ #20890]
>>     * elf/cache.c (save_cache): Call fsync on temporary file before
>>     renaming it.
>>     (save_aux_cache): Call fdatasync on temporary file before renaming
>>     it.
> 
> Ping?

LGTM.

Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>

> 
> <https://sourceware.org/ml/libc-alpha/2018-01/msg00660.html>
> 
> Thanks,
> Florian