Bug #86166 for File-Slurp: atomic write_file can delete innocent bystanders

Sat Jun 15 07:31:58 2013 debian.axhn [...] manchmal.in-ulm.de - Ticket created

Subject:	atomic write_file can delete innocent bystanders
Date:	Sat, 15 Jun 2013 13:31:37 +0200
To:	bug-File-Slurp [...] rt.cpan.org
From:	Christoph Biedl <debian.axhn [...] manchmal.in-ulm.de>

Hello, during code review of File::Slurp I noticed the temporary file name used for write_file using the "atomic" option is written without any prior checks of existance. If that file (target file with ".$$") exists, it gets lost during write_file. See the reproducer below, tested von Perl 5.14, File::Slurp 9999.19 as provided by Debian wheezy. Setting no_clobber for atomic write_file is no sane work around, this will cause random obscure failures. Please borrow or implement some techniques for unique file names as provided by File::Temp and friends instead. Cheers, Christoph #!/usr/bin/perl use 5.010; use strict; use warnings; use File::Slurp; use Test::More; my $victim = "file.$$"; write_file ($victim, ''); ok (-f $victim, 'have a victim file'); # This must not destroy the victim but does in 9999.19 write_file ('file', { 'atomic' => 1 }, 'foo'); ok (-f $victim, 'victim file still there'); done_testing;

Download signature.asc
application/pgp-signature 198b

Message body not shown because it is not plain text.

Fri Dec 19 14:47:59 2014 ether [...] cpan.org - Severity Critical added

Fri Sep 18 15:55:23 2015 gsullivan [...] cpan.org - Correspondence added

Nice test. I uploaded 3 items: - patch for Slurp.pm - patch for test error.t - new test atom1.t The patch prevents the victim file from being deleted. If the temporary file exists, the code croaks unconditionally, and the file is never written.

Subject:

atom1.t

# https://rt.cpan.org/Ticket/Display.html?id=86166 use warnings; use strict; use File::Slurp qw(write_file); use Test::More tests => 3; my $file = 'atom1.txt' ; my $victim = "$file.$$"; write_file($victim, ''); ok(-f $victim, 'have a victim file'); # This must not destroy the victim $@ = ''; eval { write_file($file, { 'atomic' => 1 }, 'foo') }; like($@, qr/atomic temporary file/, 'die if no filename'); ok(-f $victim, 'victim file still there'); unlink $victim;

Subject:

diff-pm.txt

--- ../File-Slurp-9999.19.orig/lib/File/Slurp.pm 2011-05-30 15:58:53.000000000 -0400 +++ lib/File/Slurp.pm 2015-09-18 11:20:12.294297000 -0400 @@ -446,6 +446,7 @@ # in atomic mode, we spew to a temp file so make one and save the original # file name. + _check_file($file_name); $orig_file_name = $file_name ; $file_name .= ".$$" ; } @@ -614,6 +615,7 @@ #print "EXIST [$$existing_data]\n" ; $opts->{atomic} = 1 ; + _check_file($file_name); my $write_result = eval { write_file( $file_name, $opts, $prepend_data, $$existing_data ) ; @@ -671,6 +673,7 @@ my( $edited_data ) = map { $edit_code->(); $_ } $$existing_data ; $opts->{atomic} = 1 ; + _check_file($file_name); my $write_result = eval { write_file( $file_name, $opts, $edited_data ) } ; @@ -724,6 +727,7 @@ my @edited_data = map { $edit_code->(); $_ } @$existing_data ; $opts->{atomic} = 1 ; + _check_file($file_name); my $write_result = eval { write_file( $file_name, $opts, @edited_data ) } ; @@ -807,6 +811,15 @@ return undef ; } +sub _check_file { + my $file = shift; + my $file_temp .= "$file.$$"; + if (-e $file_temp) { + # We should unconditionally die + croak("Error: atomic temporary file ($file_temp) already exists"); + } +} + 1; __END__ @@ -1066,7 +1079,8 @@ file is closed it is renamed to the original file name (and rename is an atomic operation on most OS's). If the program using this were to crash in the middle of this, then the file with the pid suffix could -be left behind. +be left behind. If the temporary file already exists, the code will +C<croak>, regardless of the C<err_mode> setting. =head3 append

Subject:

diff-test.txt

--- ../File-Slurp-9999.19.orig/t/error.t 2011-05-03 04:03:02.000000000 -0400 +++ t/error.t 2015-09-18 10:51:51.091968000 -0400 @@ -77,6 +77,7 @@ sub => \&prepend_file, args => [ $file_name ], error => qr/read_file/, + posttest => sub { unlink $file_name, "$file_name.$$" }, }, { name => 'prepend_file write error',

Fri Sep 18 15:55:23 2015 The RT System itself - Status changed from 'new' to 'open'

Sat Sep 19 03:45:21 2015 SREZIC [...] cpan.org - Correspondence added

On 2015-09-18 15:55:23, GSULLIVAN wrote: Show quoted text

> Nice test. I uploaded 3 items: > - patch for Slurp.pm > - patch for test error.t > - new test atom1.t > > The patch prevents the victim file from being deleted. If the > temporary file > exists, the code croaks unconditionally, and the file is never > written.

Even if the usage of $file.$$ is documented in File::Slurp's Pod, I think Christoph's suggestion of using a really unique temporary file is better than croaking.

Sat Sep 19 04:11:59 2015 debian.axhn [...] manchmal.in-ulm.de - Correspondence added

Subject:	Re: [rt.cpan.org #86166] atomic write_file can delete innocent bystanders
Date:	Sat, 19 Sep 2015 10:11:38 +0200
To:	Gene Sullivan via RT <bug-File-Slurp [...] rt.cpan.org>
From:	Christoph Biedl <debian.axhn [...] manchmal.in-ulm.de>

Gene Sullivan via RT wrote... Show quoted text

> The patch prevents the victim file from being deleted. If the temporary file > exists, the code croaks unconditionally, and the file is never written.

You're doing it wrong. * The user of File::Slurp must not be punished for an accidential existence of the victim file. * Probing still leaves a window for a race condition attack. So the fix should basically something like (needs some testing): +use File::Basename; +use File::Temp qw(tempfile); $orig_file_name = $file_name ; - $file_name .= ".$$" ; + my ($bn, $dn) = fileparse ($file_name); + (undef, $file_name) = tempfile ("$bn.$$.XXXXX", DIR => $dn, UNLINK => 1); (Wisdom of the elder: If you create a temporary file, leave a trace about who did this, hence "$$" in the pattern.) Additionally, it was sane to use the file handle created by tempfile instead of creating a new one. This requires a still rather small code rewrite. Christoph

Sat Sep 19 04:25:22 2015 debian.axhn [...] manchmal.in-ulm.de - Correspondence added

Subject:	Re: [rt.cpan.org #86166] atomic write_file can delete innocent bystanders
Date:	Sat, 19 Sep 2015 10:25:12 +0200
To:	Gene Sullivan via RT <bug-File-Slurp [...] rt.cpan.org>
From:	Christoph Biedl <debian.axhn [...] manchmal.in-ulm.de>

Christoph Biedl wrote... Show quoted text

> You're doing it wrong. > > * The user of File::Slurp must not be punished for an accidential > existence of the victim file. > * Probing still leaves a window for a race condition attack.

And third, File::Slurp has err_mode which should be respected. My proposal didn't either, tempfile can fail although I've never experienced this in many years. So it has to be eval'd, plus error handling. Christoph

Thu Sep 24 16:53:17 2015 uri [...] stemsystems.com - Correspondence added

Subject:	Re: [rt.cpan.org #86166] atomic write_file can delete innocent bystanders
Date:	Thu, 24 Sep 2015 16:50:17 -0400
To:	bug-File-Slurp [...] rt.cpan.org
From:	Uri Guttman <uri [...] stemsystems.com>

On 09/19/2015 04:25 AM, Christoph Biedl via RT wrote: Show quoted text

> Queue: File-Slurp > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=86166 > > > Christoph Biedl wrote... >

>> You're doing it wrong. >> >> * The user of File::Slurp must not be punished for an accidential >> existence of the victim file. >> * Probing still leaves a window for a race condition attack.

> And third, File::Slurp has err_mode which should be respected. My > proposal didn't either, tempfile can fail although I've never > experienced this in many years. So it has to be eval'd, plus error > handling.

you are all missing some major points. the reason for atomic mode is to make sure the new file is completely written before it replaces the existing file. this will ensure any reader of the file will see consistent data (assuming your writer generates it correctly). the issue of a race condition isn't covered as that should be done with a lock file anyway. if you use the no_clobber option to write_file it sets the O_EXCL flag for sysopen. this will fail if that temp file exists. any error there can be handled by slurp's standard error code. there is no need for a special temp file name. one case which isn't covered is the race between writing the file and the rename call. if the process crashes then, the temp file could be left around. an unlink on the file after it is created would eliminate that but even then, a crash before the unlink would leave it. races are races. if a user has overwritten a file, that is on their head as that is what lock files and similar things are for. i can't lookout for every stupid coder mistake in using slurp. the module does what it does and pretty well and popular. my day job site uses it all over the place as do i. if you want more safety you have to handle it yourself which is true with many modules and code in general. thanx, uri

Fri Sep 25 13:50:45 2015 SREZIC [...] cpan.org - Correspondence added

On 2015-09-24 16:53:17, uri@stemsystems.com wrote: Show quoted text

> > On 09/19/2015 04:25 AM, Christoph Biedl via RT wrote:

> > Queue: File-Slurp > > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=86166 > > > > > Christoph Biedl wrote... > >

> >> You're doing it wrong. > >> > >> * The user of File::Slurp must not be punished for an accidential > >> existence of the victim file. > >> * Probing still leaves a window for a race condition attack.

> > And third, File::Slurp has err_mode which should be respected. My > > proposal didn't either, tempfile can fail although I've never > > experienced this in many years. So it has to be eval'd, plus error > > handling.

> you are all missing some major points. the reason for atomic mode is to > make sure the new file is completely written before it replaces the > existing file. this will ensure any reader of the file will see > consistent data (assuming your writer generates it correctly). the issue > of a race condition isn't covered as that should be done with a lock > file anyway. > > if you use the no_clobber option to write_file it sets the O_EXCL flag > for sysopen. this will fail if that temp file exists. any error there > can be handled by slurp's standard error code. there is no need for a > special temp file name. one case which isn't covered is the race between > writing the file and the rename call. if the process crashes then, the > temp file could be left around. an unlink on the file after it is > created would eliminate that but even then, a crash before the unlink > would leave it. races are races. > > if a user has overwritten a file, that is on their head as that is what > lock files and similar things are for. i can't lookout for every stupid > coder mistake in using slurp. the module does what it does and pretty > well and popular. my day job site uses it all over the place as do i. if > you want more safety you have to handle it yourself which is true with > many modules and code in general.

So you suggest that a defensive programmer should rewrite the write_file() call above like this? my $content = 'foo'; my $filename = 'file'; eval { write_file ($filename, { 'atomic' => 1, no_clobber => 1 }, $content); }; if ($@) { # It would be probably better to check using $!{EEXISTS}, # but is it for sure that $! is unclobbered since the # sysopen call? Otherwise one would find a way to # handle the locale-specific messages, maybe using # POSIX::strerror. if ($@ =~ /(Die Datei existiert bereits|File exists)/) { require File::Basename; require File::Temp; my $tmp = File::Temp->new(DIR => File::Basename::dirname($filename)); $tmp->print($content); $tmp->close or die $!; rename $tmp, $filename or die $!; } else { die $@; } }

Fri Sep 25 20:32:23 2015 uri [...] stemsystems.com - Correspondence added

Subject:	Re: [rt.cpan.org #86166] atomic write_file can delete innocent bystanders
Date:	Fri, 25 Sep 2015 20:32:12 -0400
To:	bug-File-Slurp [...] rt.cpan.org
From:	Uri Guttman <uri [...] stemsystems.com>

On 09/25/2015 01:50 PM, Slaven_Rezic via RT wrote: Show quoted text

> Queue: File-Slurp > So you suggest that a defensive programmer should rewrite the write_file() call above like this? > >

no, i said use a lock file which you would use anyway if you need to make sure a file is properly written and stable. slurp does not do any locking and isn't meant for that sort of safety. you can't eliminate race conditions without an external lock or similar. hell, the file system may not even be flushed yet when a system crash happens. you want safety, you have to code for safety in whatever way you like. eval on slurp still won't handle all known race conditions like a system crash. to avoid that, you need to double write or use a logging file system or a DB with ACID compliance. it all depends on how serious you are about having stable files on disk. write_file is good enough with atomic and noclobber for most cases. i can't and won't fix it for race conditions as that is out of scope for the module's purpose. uri

Sat Sep 26 02:32:28 2015 SREZIC [...] cpan.org - Correspondence added

On 2015-09-25 20:32:23, uri@stemsystems.com wrote: Show quoted text

> On 09/25/2015 01:50 PM, Slaven_Rezic via RT wrote:

> > Queue: File-Slurp > > So you suggest that a defensive programmer should rewrite the > > write_file() call above like this? > > > >

> > no, i said use a lock file which you would use anyway if you need to > make sure a file is properly written and stable. slurp does not do any > locking and isn't meant for that sort of safety. you can't eliminate > race conditions without an external lock or similar. hell, the file > system may not even be flushed yet when a system crash happens. you > want > safety, you have to code for safety in whatever way you like. eval on > slurp still won't handle all known race conditions like a system > crash. > to avoid that, you need to double write or use a logging file system > or > a DB with ACID compliance. it all depends on how serious you are about > having stable files on disk. write_file is good enough with atomic and > noclobber for most cases. i can't and won't fix it for race conditions > as that is out of scope for the module's purpose.

How would lock files solve the original issue? To recapitulate: A file named "$base.$integer" already exists, for whatever reason. A process running with pid $integer wants to atomically write a file named $base. This is currently not possible with write_file --- without no_clobber it would delete the existing file, and with no_clobber write_file would fail.

Wed Feb 13 11:45:43 2019 cwhitener [...] gmail.com - Taken

Wed Feb 13 11:47:27 2019 cwhitener [...] gmail.com - Correspondence added

Hi Everyone, Version 9999.26 has just been released and the original issue here of files possibly being clobbered has been resolved by using File::Temp. I'm closing this out now as I believe it to be resolved, but please yell if I'm wrong! Thanks, Chase

Wed Feb 13 11:47:28 2019 cwhitener [...] gmail.com - Status changed from 'open' to 'resolved'