Bug #20432 for Authen-Captcha: Broken locking lets images files build up

Original mail to authors: =================================================== We recently started using Authen::Captcha (1.23). I noticed that within a few days of starting to use it, tens of thousands of image files had built up, which were not being expired, and were nowhere to be found in the database of codes. This was on busy dual-processor boxes, but I managed to reproduce it on my own workstation easily: ab -c3 -n1000 http://some/url/which/generates/captchas By the time 500 or so requests had passed, I had fairly consistently "lost" a few images from the database. The debug output showed that older versions of the data were overwriting more recent versions. The problem is that one has to obtain a lock, read in the data file, do everything that you're going to do with it, and then write it back. Instead, Authen::Captcha was reading in the data file, then closing it and releasing the lock, then modifying the data (in which time another process could be locking and reading in the file, obtaining a stale copy of the data), then re-locking and re-opening it, and so on. Attached is a solution (tested quite extensively and free of the problem), as well as the original file, and a diff. You may want to go about the locking differently - I chose to lock on an external file rather than the database itself, both being paranoid about the caching issues one can read about in Locking: The Trouble with fd (man DB_File) (which may not apply here - not sure), and also believing that it's cleaner/simpler. Note also that in the attached, Captcha.pm the locks _have_ to be exclusive, not shared, or the same bug would be reintroduced. Anyway, I'm hoping that you still maintain this module - I see that there have been no new releases for a long time. Please let me know. ===================================================

--- Captcha.pm.orig 2006-07-05 17:29:37.000000000 +0200 +++ Captcha.pm 2006-07-06 10:52:26.000000000 +0200 @@ -218,8 +218,8 @@ # pull in current database warn "Open File: $database_file\n" if($self->debug() >= 2); + $self->_get_exclusive_lock(); open (DATA, "<$database_file") or die "Can't open File: $database_file\n"; - flock DATA, 1; # read lock my @data=<DATA>; close(DATA); warn "Close File: $database_file\n" if($self->debug() >= 2); @@ -285,13 +285,44 @@ # update database open(DATA,">$database_file") or die "Can't open File: $database_file\n"; - flock DATA, 2; # write lock print DATA $new_data; close(DATA); + $self->_release_lock(); return $return_value; } + +sub _open_lock_file { + my $self = shift; + my $file_name = shift; + open(LOCK, ">>$file_name") or die "Error opening lockfile $file_name: $!\n"; +} + +sub _get_shared_lock { + my $self = shift; + my $lock_file_name = File::Spec->catfile($self->data_folder(),"codes.lock"); + $self->_open_lock_file($lock_file_name); + + # shared lock + flock(LOCK, 1) or die "Error locking lockfile in shared mode: $!\n"; +} + +sub _get_exclusive_lock { + my $self = shift; + my $lock_file_name = File::Spec->catfile($self->data_folder(),"codes.lock"); + $self->_open_lock_file($lock_file_name); + + # exclusive lock + flock(LOCK, 2) or die "Error locking lockfile exclusively: $!\n"; +} + +sub _release_lock { + my $self = shift; + flock(LOCK, 8) or die "Error unlocking lockfile: $!\n"; + close(LOCK); +} + sub _touch_file { ref(my $self = shift) or croak "instance variable needed"; @@ -342,8 +373,9 @@ $self->_touch_file($database_file); # clean expired codes and images + $self->_get_exclusive_lock(); + open (DATA, "<$database_file") or die "Can't open File: $database_file\n"; - flock DATA, 1; # read lock my @data=<DATA>; close(DATA); @@ -365,12 +397,12 @@ # save the code to database warn "open File: $database_file\n" if($self->debug() >= 2); open(DATA,">$database_file") or die "Can't open File: $database_file\n"; - flock DATA, 2; # write lock warn "-->>" . $new_data . "\n" if($self->debug() >= 2); warn "-->>" . $current_time . "::" . $md5."\n" if($self->debug() >= 2); print DATA $new_data; print DATA $current_time."::".$md5."\n"; close(DATA); + $self->_release_lock(); warn "Close File: $database_file\n" if($self->debug() >= 2); }