Skip Menu |
 

This queue is for tickets about the File-Slurp CPAN distribution.

Report information
The Basics
Id: 86166
Status: open
Priority: 0/
Queue: File-Slurp

People
Owner: Nobody in particular
Requestors: debian.axhn [...] manchmal.in-ulm.de
Cc:
AdminCc:

Bug Information
Severity: Critical
Broken in: (no value)
Fixed in: (no value)



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>
Download (untitled) / with headers
text/plain 934b
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.

Download (untitled) / with headers
text/plain 248b
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
Download atom1.t
text/x-perl 475b
# 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
Download diff-pm.txt
text/plain 1.6k
--- ../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
Download diff-test.txt
text/plain 332b
--- ../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',
Download (untitled) / with headers
text/plain 469b
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.
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>
Download (untitled) / with headers
text/plain 959b
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
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>
Download (untitled) / with headers
text/plain 440b
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
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>
Download (untitled) / with headers
text/plain 1.8k
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
Download (untitled) / with headers
text/plain 2.7k
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 $@; } }
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
Download (untitled) / with headers
text/plain 1.4k
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.


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.