Skip to content

Fix #280 - open_buffer mangles the content of the buffer it is given. #360

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Sep 5, 2019
Merged

Fix #280 - open_buffer mangles the content of the buffer it is given. #360

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7cd263e
Clean up use of file_name in Zip::File::initialize.
hainesr Apr 3, 2018
cfa9441
Handle passing an IO to Zip::File.new better.
hainesr Apr 3, 2018
0363393
No need to require stringio in Zip::File.open_buffer.
hainesr Apr 4, 2018
15ccc25
Fix File.open_buffer when no changes are made.
hainesr Apr 4, 2018
84c2089
Switch newly created StringIOs to binmode.
hainesr Apr 4, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 30 additions & 17 deletions lib/zip/file.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,24 +64,38 @@ class File < CentralDirectory

# Opens a zip archive. Pass true as the second parameter to create
# a new archive if it doesn't exist already.
def initialize(file_name, create = false, buffer = false, options = {})
def initialize(path_or_io, create = false, buffer = false, options = {})
super()
@name = file_name
@name = path_or_io.respond_to?(:path) ? path_or_io.path : path_or_io
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is good... but it seems a bit optional. Was it aimed at solving a particular problem?

@comment = ''
@create = create ? true : false # allow any truthy value to mean true
if !buffer && ::File.size?(file_name)

if ::File.size?(@name.to_s)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If @name is a StringIO, @name.to_s will return something like "#<StringIO:0x00007f98b09766c0>". It's unlikely that such a file exists, of course, but hitting the file system to find this out seems a bit odd. Could it be reorganised to not check the file system at all when buffer is true?

# There is a file, which exists, that is associated with this zip.
@create = false
@file_permissions = ::File.stat(file_name).mode
::File.open(name, 'rb') do |f|
read_from_stream(f)
@file_permissions = ::File.stat(@name).mode

if buffer
read_from_stream(path_or_io)
else
::File.open(@name, 'rb') do |f|
read_from_stream(f)
end
end
elsif buffer && path_or_io.size > 0
# This zip is probably a non-empty StringIO.
read_from_stream(path_or_io)
elsif @create
# This zip is completely new/empty and is to be created.
@entry_set = EntrySet.new
elsif ::File.zero?(file_name)
raise Error, "File #{file_name} has zero size. Did you mean to pass the create flag?"
elsif ::File.zero?(@name)
# A file exists, but it is empty.
raise Error, "File #{@name} has zero size. Did you mean to pass the create flag?"
else
raise Error, "File #{file_name} not found"
# Everything is wrong.
raise Error, "File #{@name} not found"
end

@stored_entries = @entry_set.dup
@stored_comment = @comment
@restore_ownership = options[:restore_ownership] || false
Expand Down Expand Up @@ -119,17 +133,16 @@ def open_buffer(io, options = {})
unless IO_METHODS.map { |method| io.respond_to?(method) }.all? || io.is_a?(String)
raise "Zip::File.open_buffer expects a String or IO-like argument (responds to #{IO_METHODS.join(', ')}). Found: #{io.class}"
end
if io.is_a?(::String)
require 'stringio'
io = ::StringIO.new(io)
elsif io.respond_to?(:binmode)
# https://github.com/rubyzip/rubyzip/issues/119
io.binmode
end

io = ::StringIO.new(io) if io.is_a?(::String)

# https://github.com/rubyzip/rubyzip/issues/119
io.binmode if io.respond_to?(:binmode)

zf = ::Zip::File.new(io, true, true, options)
zf.read_from_stream(io)
return zf unless block_given?
yield zf

begin
zf.write_buffer(io)
rescue IOError => e
Expand Down
12 changes: 10 additions & 2 deletions test/file_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,13 @@ def test_get_output_stream
end
end

def test_open_buffer_with_string
string = File.read('test/data/rubycode.zip')
::Zip::File.open_buffer string do |zf|
assert zf.entries.map { |e| e.name }.include?('zippedruby1.rb')
end
end

def test_open_buffer_with_stringio
string_io = StringIO.new File.read('test/data/rubycode.zip')
::Zip::File.open_buffer string_io do |zf|
Expand All @@ -107,13 +114,14 @@ def test_open_buffer_with_stringio
def test_close_buffer_with_stringio
string_io = StringIO.new File.read('test/data/rubycode.zip')
zf = ::Zip::File.open_buffer string_io
assert(zf.close || true) # Poor man's refute_raises
assert_nil zf.close
end

def test_close_buffer_with_io
f = File.open('test/data/rubycode.zip')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: this is similar (but not identical) to the test cases in #146 and #192, so they can probably be closed once we sort this out.

zf = ::Zip::File.open_buffer f
assert zf.close
refute zf.commit_required?
assert_nil zf.close
f.close
end

Expand Down