JRuby

JRuby flock silently converts LOCK_EX to LOCK_SH on read-only files

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Fixed
  • Affects Version/s: JRuby 1.6
  • Fix Version/s: JRuby 1.6.1
  • Component/s: Core Classes/Modules
  • Labels:
    None
  • Environment:
    Solaris 10
  • Number of attachments :
    1

Description

In JRUBY-1214 the behavior of File.flock was changed to silently convert LOCK_EX locks on files opened in read mode into LOCK_SH locks. This was because some operating systems allow setting LOCK_EX on files opened for read only. However, this is not specified in POSIX. In fact, on Solaris, when trying to get an exclusive lock on a file opened only for read, the OS will return EBADF. So this isn't really a MRI vs JVM issue, but an OS issue. Any code that depends on this behavior in MRI is not portable.

All of that aside, this violates the principle of least surprise. No one asking for an exclusive lock is going to be happy when their lock is silently converted to a shared lock, and this could very easily lead to silent data corruption.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Charles Oliver Nutter added a comment -

Hmm, I tend to agree with you. flock should be locking exclusively; that's the implication from the API. I think what we should do rather than convert to a shared lock is capture the NIO error and raise it as something like an argument error indicating you can't flock a read-only file.

Show
Charles Oliver Nutter added a comment - Hmm, I tend to agree with you. flock should be locking exclusively; that's the implication from the API. I think what we should do rather than convert to a shared lock is capture the NIO error and raise it as something like an argument error indicating you can't flock a read-only file.
Hide
Permalink
Charles Oliver Nutter added a comment -

I've attached a patch that mimics the JDK behavior for both EX (must be writable) and SH (must be readable).

Could you confirm what MRI does for both cases on Solaris? I'm not sure whether we should be raising EBADF or just returning a zero result.

We'll also need to investigate the RubySpecs for flock, since they open a file only for read and then try to acquire exclusive locks against it.

Show
Charles Oliver Nutter added a comment - I've attached a patch that mimics the JDK behavior for both EX (must be writable) and SH (must be readable). Could you confirm what MRI does for both cases on Solaris? I'm not sure whether we should be raising EBADF or just returning a zero result. We'll also need to investigate the RubySpecs for flock, since they open a file only for read and then try to acquire exclusive locks against it.
Hide
Permalink
Clayton O'Neill added a comment -

Test script:

file_modes = [ "r", "r+", "w" ]                                                                     
lock_modes = {                                                                                      
  'LOCK_EX' => File::LOCK_EX,                                                                       
  'LOCK_SH' => File::LOCK_SH                                                                        
}                                                                                                   
                                                                                                    
file_modes.each do |file_mode|                                                                      
  lock_modes.each do |lock_name, lock_mode|                                                         
    f = File.open(ARGV[0], File::RDWR|File::CREAT, 0644)                                            
    f.close                                                                                         
                                                                                                    
    f = File.open(ARGV[0], file_mode)                                                               
    begin                                                                                           
      if f.flock(File::LOCK_NB | lock_mode)                                                         
        puts "lock: #{lock_name} mode: #{file_mode} - granted"                                      
      else                                                                                          
        puts "lock: #{lock_name} mode: #{file_mode} - would block"                                  
      end                                                                                           
    rescue                                                                                          
      puts "lock: #{lock_name} mode: #{file_mode} - exception \"#{$!}\""                            
    end                                                                                             
    f.close                                                                                         
    File.unlink(ARGV[0])                                                                            
  end                                                                                               
end

Output on Solaris:

lock: LOCK_EX mode: r - exception "Bad file number - /tmp/test"
lock: LOCK_SH mode: r - granted
lock: LOCK_EX mode: r+ - granted
lock: LOCK_SH mode: r+ - granted
lock: LOCK_EX mode: w - granted
lock: LOCK_SH mode: w - exception "Bad file number - /tmp/test"
Show
Clayton O'Neill added a comment - Test script: file_modes = [ "r", "r+", "w" ] lock_modes = { 'LOCK_EX' => File::LOCK_EX, 'LOCK_SH' => File::LOCK_SH } file_modes.each do |file_mode| lock_modes.each do |lock_name, lock_mode| f = File.open(ARGV[0], File::RDWR|File::CREAT, 0644) f.close f = File.open(ARGV[0], file_mode) begin if f.flock(File::LOCK_NB | lock_mode) puts "lock: #{lock_name} mode: #{file_mode} - granted" else puts "lock: #{lock_name} mode: #{file_mode} - would block" end rescue puts "lock: #{lock_name} mode: #{file_mode} - exception \"#{$!}\"" end f.close File.unlink(ARGV[0]) end end Output on Solaris: lock: LOCK_EX mode: r - exception "Bad file number - /tmp/test" lock: LOCK_SH mode: r - granted lock: LOCK_EX mode: r+ - granted lock: LOCK_SH mode: r+ - granted lock: LOCK_EX mode: w - granted lock: LOCK_SH mode: w - exception "Bad file number - /tmp/test"
Hide
Permalink
Charles Oliver Nutter added a comment -

So just to confirm, those "Bad file number" errors actually are EBADF exceptions, yes? I'll try to get on a Solaris instance today to confirm.

Show
Charles Oliver Nutter added a comment - So just to confirm, those "Bad file number" errors actually are EBADF exceptions, yes? I'll try to get on a Solaris instance today to confirm.
Hide
Permalink
Clayton O'Neill added a comment -

Yeap, they're Errno::EBADF

Show
Clayton O'Neill added a comment - Yeap, they're Errno::EBADF
Hide
Permalink
Charles Oliver Nutter added a comment -

commit a3e44c84176f6e1e7b97386b3a699c8c6ccce3cc
Author: Charles Oliver Nutter <headius@headius.com>
Date: Fri Mar 25 20:43:16 2011 -0500

Fix JRUBY-5627: JRuby flock silently converts LOCK_EX to LOCK_SH on read-only files

Show
Charles Oliver Nutter added a comment - commit a3e44c84176f6e1e7b97386b3a699c8c6ccce3cc Author: Charles Oliver Nutter <headius@headius.com> Date: Fri Mar 25 20:43:16 2011 -0500 Fix JRUBY-5627 : JRuby flock silently converts LOCK_EX to LOCK_SH on read-only files
Hide
Permalink
Charles Oliver Nutter added a comment - - edited

FYI, Rails is one case that appeared to be attempting to lock read-only files exclusively. I have filed a bug: https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/6662-fileflock-cant-lock-read-only-file-for-exclusive-access

Show
Charles Oliver Nutter added a comment - - edited FYI, Rails is one case that appeared to be attempting to lock read-only files exclusively. I have filed a bug: https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/6662-fileflock-cant-lock-read-only-file-for-exclusive-access

People

  • Assignee:
    Charles Oliver Nutter
    Reporter:
    Clayton O'Neill
Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved: