Skip Menu |
 

Preferred bug tracker

Please visit the preferred bug tracker to report your issue.

This queue is for tickets about the CGI CPAN distribution.

Report information
The Basics
Id: 92332
Status: resolved
Priority: 0/
Queue: CGI

People
Owner: Nobody in particular
Requestors: killing [...] multiplay.co.uk
Cc:
AdminCc:

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



Subject: Nasty race condition in tempory file handling
Date: Tue, 21 Jan 2014 13:00:19 -0000
To: <bug-CGI [...] rt.cpan.org>
From: "Steven Hartland" <killing [...] multiplay.co.uk>
Download (untitled) / with headers
text/plain 2.8k
The following code has nasty race condition which can cause problems with multiple simultaneous file uploads. # choose a relatively unpredictable tmpfile sequence number my $seqno = unpack("%16C*",join('',localtime,grep {defined $_} values %ENV)); for (my $cnt=10;$cnt>0;$cnt--) { next unless $tmpfile = CGITempFile->new($seqno); $tmp = $tmpfile->as_string; last if defined($filehandle = Fh->new($filename,$tmp,$PRIVATE_TEMPFILES)); $seqno += int rand(100); } Here CGITempFile creates a file based on checks for which it believes is a valid new and unique temporary filename. Fh then tries to create said file but can fail if the file already exists e.g. when two uploads are executing at the same time. If a Fh does fail detect a conflict another iteration of the loop occurs to look for a new name. At this point the previous loop iterations CGITempFile goes out of scope which triggers its DESTORY which unlinks the file this process never opened. Due to this unlink a third process can then allocate the same filename as process #1. Now if process #1 does anything with the filename it can access via $cgi->tmpFileName it will access the file created by #3 which is not the intention. Ideally CGITempFile should be responsible for the creation of the file, so the details it returns (which should include a filehandle) are guaranteed to work. Alternatively an additional method could be added to CGITempFile to prevent it deleting the file so the following something like the following code (untested) could be used. for (my $cnt=10;$cnt>0;$cnt--) { next unless $tmpfile = CGITempFile->new($seqno); $tmp = $tmpfile->as_string; last if defined($filehandle = Fh->new($filename,$tmp,$PRIVATE_TEMPFILES)); # Tell CGITempFile that the filename is invalid so it # should not perform an unlink on delete $tmpfile->unused(); $seqno += int rand(100); } sub unused { my ($self) = @_ $$self = ''; } sub DESTROY { my($self) = @_; return if ($$safe eq ''); # filename wasn't unused $$self =~ m!^([a-zA-Z0-9_ \'\":/.\$\\~-]+)$! || return; my $safe = $1; # untaint operation unlink $safe; # get rid of the file } Finally it may be a good idea to update $seqno to generate more random values as the current method virtually always results in clashes for multiple simultaneous processes. The following produces a pretty random number: my $seqno = (localtime())[0] . $$; Regards Steve ================================================ This e.mail is private and confidential between Multiplay (UK) Ltd. and the person or entity to whom it is addressed. In the event of misdirection, the recipient is prohibited from using, copying, printing or otherwise disseminating it or any information contained in it. In the event of misdirection, illegible or incomplete transmission please telephone +44 845 868 1337 or return the E.mail to postmaster@multiplay.co.uk.
Download (untitled) / with headers
text/plain 241b
This issue has been copied to: https://github.com/leejo/CGI.pm/issues/131 please take all future correspondence there. This ticket will remain open but please do not reply here. This ticket will be closed when the github issue is dealt with.
Download (untitled) / with headers
text/plain 1.4k
commit 1aa4cb0262103d7d0f657099bb54a557ce1b237b Author: Lee Johnson <lee@givengain.ch> Date: Sun Sep 28 20:46:46 2014 +0100 resolve #125, #131 - refactor temp file handling the CGITempFile and Fh packages are gone and have been replaced by CGI::File::Temp, which is a compatibility wrapper around the File::Temp package. File::Temp behaves as a filehandle, and isa IO::Handle and isa IO::Seekable so behaves as we want it to bump version to 4.04_02 for cpantesters dev release testing Squashed commit of the following: commit 3af477dacef09d7d01086a6fb3984471e6892c9e Author: Lee Johnson <lee@givengain.ch> Date: Sun Sep 28 20:45:32 2014 +0100 temp files done commit 982fc37665a8754d09e7f5952bc04a736ebcfc26 Author: Lee Johnson <lee@givengain.ch> Date: Thu Sep 25 10:55:33 2014 +0100 WIP - refactor CGITempFile and Fh to File::Temp Changes | 8 ++-- MANIFEST | 1 - Makefile.PL | 19 ++++----- lib/CGI.pm | 328 ++++++++++++++++++++++++++++++++++------------------------------------------------------------------------------------------------------------------------- lib/CGI/Carp.pm | 2 +- lib/CGI/Cookie.pm | 2 +- lib/CGI/Pretty.pm | 2 +- lib/CGI/Push.pm | 2 +- lib/CGI/Util.pm | 2 +- t/rt-31107.t | 12 +++++- t/tmpdir.t | 51 ------------------------ t/upload.t | 2 +- 12 files changed, 103 insertions(+), 328 deletions(-)


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.