JRuby (please use github issues at http://bugs.jruby.org)
  1. JRuby (please use github issues at http://bugs.jruby.org)
  2. JRUBY-5627

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

        Hide
        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
        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
        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
        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
        Clayton O'Neill added a comment -

        Yeap, they're Errno::EBADF

        Show
        Clayton O'Neill added a comment - Yeap, they're Errno::EBADF
        Hide
        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
        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
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: