ruby-core

I tend to fork off long-running processes from CGI scripts.  The
problem with these is that unless I specifically remember to close the
CGI::Session file, the broken locking logic in the FileStore class
prevents a user who just caused one of those processes to be forked
off to be stuck waiting on a lock that shouldn't be a problem at all.

I've proposed a patch to fix the locking problem on ruby-talk several
times, but got no response at all.  I'm hoping at least here someone
will tell me why I'm wrong to do it this way.  At best, I'd like to
see this go into 1.8.2.

Key features:

1) FileStore doesn't need to hold an open filehandle all the time.
   This can cause several race conditions when locking files anyway.
   I've removed this, and only open a file when I actually need to
   read from or write to it.  I can't see any real benefit to holding
   it open all the time.

2) Locks were granted, but never released.  By closing the file after
   use, this releases the lock and also has the added benefit of
   reducing the possibility for race conditions.

3) Restore used LOCK_EX when reading the file-- this is generally
   regarded as incorrect.  Reading processes can work fine with shared
   locks; only writers need exclusive locks.

Here's the patch (to 1.8.1):

--- session-old.rb	2004-01-26 17:24:25.000000000 -0700
+++ session.rb	2004-01-26 17:33:40.000000000 -0700
@@ -364,16 +364,13 @@
 	unless check_id(id)
 	  raise ArgumentError, "session_id `%s' is invalid" % id
 	end
-	path = dir+"/"+prefix+id
-	path.untaint
-	unless File::exist? path
+	@path = dir+"/"+prefix+id
+	@path.untaint
+	if File::exist? @path
+	  restore
+	else
 	  @hash = {}
 	end
-	begin
-	  @f = open(path, "r+")
-	rescue Errno::ENOENT
-	  @f = open(path, "w+")
-	end
       end
 
       # Restore session state from the session's FileStore file.
@@ -382,13 +379,17 @@
       def restore
 	unless @hash
 	  @hash = {}
-	  @f.flock File::LOCK_EX
-	  @f.rewind
-	  for line in @f
-	    line.chomp!
-	    k, v = line.split('=',2)
-	    @hash[CGI::unescape(k)] = CGI::unescape(v)
-	  end
+          begin
+	    f = File.open(@path, 'r')
+	    f.flock File::LOCK_SH
+	    for line in f
+	      line.chomp!
+	      k, v = line.split('=',2)
+	      @hash[CGI::unescape(k)] = CGI::unescape(v)
+	    end
+          ensure
+	    f.close unless f.nil?
+          end
 	end
 	@hash
       end
@@ -396,25 +397,25 @@
       # Save session state to the session's FileStore file.
       def update
 	return unless @hash
-	@f.rewind
-	for k,v in @hash
-	  @f.printf "%s=%s\n", CGI::escape(k), CGI::escape(String(v))
-	end
-	@f.truncate @f.tell
+        begin
+	  f = File.open(@path, 'w')
+	  f.lock File::LOCK_EX
+   	  for k,v in @hash
+	    f.printf "%s=%s\n", CGI::escape(k), CGI::escape(String(v))
+	  end
+        ensure
+          f.close unless f.nil?
+        end
       end
 
       # Update and close the session's FileStore file.
       def close
-	return if @f.closed?
 	update
-	@f.close
       end
 
       # Close and delete the session's FileStore file.
       def delete
-	path = @f.path
-	@f.close
-	File::unlink path
+	File::unlink @path
       end
     end

--  
Eric Schwartz					email: emschwar@hp.com
Hewlett-Packard Company         		Phone: +1 970 898 7443
Linux and Open Source Lab (LOSL)

Thread

Prev Next

In This Thread

Prev Next