weshatheleopard / rubyXL

Ruby lib for reading/writing/modifying .xlsx and .xlsm files
MIT License
1.27k stars 253 forks source link

`#parse_buffer` unexpectedly modifies input buffer #453

Open HosokawaR opened 5 months ago

HosokawaR commented 5 months ago

Thank you for creating such a wonderful library.

I noticed that RubyXL::Parser.parse_buffer implicitly changes the buffer received as an argument. But considering the name #parse_buffer, I don't think the passed argument buffer should be modifed.

Below is the minimal example. buffer has been modified and increased in size.

$ gem list | grep rubyXL
rubyXL (3.4.26)

$ irb
irb(main):001> require 'rubyXL'
irb(main):002> buffer = File.read("./sample.xlsx")
irb(main):003> buffer.size
=> 213663
irb(main):004> RubyXL::Parser.parse_buffer(buffer)
irb(main):005> buffer.size
=> 272845  # buffer size changed !

The cause was that when Zip::File.open_buffer was called with block, the supplied argument buffer was changed. https://github.com/rubyzip/rubyzip/blob/73c8e110ed1dbcff08ffa48bb1b094abd0348502/lib/zip/file.rb#L122

I think we can fix this problem by not using block.

→ I also created a PR. https://github.com/weshatheleopard/rubyXL/pull/454

It's a minor problem, but I get it sometimes, so I think it would be nice if it could be fixed. In my case, I discovered this problem because when I attached the parsed Excel file to an email, the Excel file was corrupted.

weshatheleopard commented 5 months ago

You see, I'm temped to NOT do anything about this issue in RubyXL since all that it does is passing the buffer to RubyZip; from that point, it's RubyZip responsebility: if it modifies buffer, it's it's fault, not RubyXLs. I do not appreciate this behavior, as there's no reason why it should ever modify the buffer, but that's what they do. I figured out that when I pass a String to RubyZip, then .freeze'ing it before passing it over does the trick. I don't have a problem adding a warning about that workaround to the documentation. But I think RubyZip's issues need to be handled with its developers. See the discussion here.

HosokawaR commented 5 months ago

I confirmed that this issue is resolved with rubyzip 2.4.rc1. https://github.com/rubyzip/rubyzip/issues/280#issuecomment-2043040338

I hope to close this issue with an rubyzip official release and an upgrade of rubyzip's dependencies.

weshatheleopard commented 5 months ago

Yes, I tried to update gems but rubyzip is not showing up as updated. I assume the reason is that it's RC and not released yet. Once it is, I will update dependencies.

weshatheleopard commented 3 days ago

It has been nearly 5 months but the latest version on Rubygems is still 2.3.2?