Patch #35720

Defect: Binmode specified twice

Added by Pavel Rosický 4 months ago. Updated 4 months ago.

Status:ClosedStart date:
Priority:NormalDue date:
Assignee:Go MAEDA% Done:

0%

Category:Attachments
Target version:-

Description

after #35539 I'm getting failures

ArgumentError: binmode specified twice
    app/models/attachment.rb:572:in `initialize'
    app/models/attachment.rb:572:in `open'
    app/models/attachment.rb:572:in `create_diskfile'
    app/models/attachment.rb:142:in `files_to_final_location'

File::BINARY flag should be enough to setup a binary mode

diff --git a/app/models/attachment.rb b/app/models/attachment.rb
index c65c56722..b0b0a02ba 100644
--- a/app/models/attachment.rb
+++ b/app/models/attachment.rb
@@ -572,7 +572,6 @@ class Attachment < ActiveRecord::Base
         File.open(
           File.join(path, name),
           flags: File::CREAT | File::EXCL | File::BINARY | File::WRONLY,
-          binmode: true,
           &block
         )
       rescue Errno::EEXIST

Related issues

Related to Redmine - Patch #35539: Race condition (possible filename collision) in Attachme... Closed

Associated revisions

Revision 21193
Added by Go MAEDA 4 months ago

Fix that binmode specified twice (#35539, #35720).

Patch by Pavel Rosický.

History

#1 Updated by Go MAEDA 4 months ago

I think what you pointed out is right but I cannot reproduce the ArgumentError by uploading files or running the test. The following code also doesn't cause errors.

File.open('/tmp/test.txt', flags: File::CREAT | File::EXCL | File::BINARY | File::WRONLY, binmode: true) {|io| io.puts 'hello'}

Could you let me know how I can reproduce the error?

#2 Updated by Pavel Rosický 4 months ago

your example fails as-well, but it depends on the platform
on Windows

File::BINARY
=> 32768

on Linux

File::BINARY
=> 0

which means it actually has no effect on Linux and it won't break if you specify the binmode argument twice.

this is because *nix operating systems don't do automatic linefeed conversion for I/O on "text" files so O_TEXT and O_BINARY flags wouldn't make sense.

#3 Updated by Pavel Rosický 4 months ago

I removed the flag instead, because binmode also affects encoding on Linux. This passes on both platforms.

diff --git a/app/models/attachment.rb b/app/models/attachment.rb
index c65c56722..8fba26eff 100644
--- a/app/models/attachment.rb
+++ b/app/models/attachment.rb
@@ -571,7 +571,7 @@ class Attachment < ActiveRecord::Base
         name = "#{timestamp}_#{ascii}" 
         File.open(
           File.join(path, name),
-          flags: File::CREAT | File::EXCL | File::BINARY | File::WRONLY,
+          flags: File::CREAT | File::EXCL | File::WRONLY,
           binmode: true,
           &block
         )

#4 Updated by Go MAEDA 4 months ago

  • Related to Patch #35539: Race condition (possible filename collision) in Attachment.disk_filename added

#5 Updated by Mischa The Evil 4 months ago

  • Description updated (diff)

#6 Updated by Go MAEDA 4 months ago

  • Status changed from New to Closed
  • Assignee set to Go MAEDA

Committed the fix as a part of #35539. Thank you.

Also available in: Atom PDF