Skip to content

Fix restore_times option when extracting, and test file options #413

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
Oct 27, 2019
Merged

Fix restore_times option when extracting, and test file options #413

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
2534575
Correctly set/default options in the File class.
hainesr Sep 16, 2019
3782935
Make the attr_accessors in File more readable.
hainesr Sep 16, 2019
8c694d3
Add functionality to restore file timestamps.
hainesr Oct 6, 2019
2bdd37d
Add a convenience method for creating DOSTime instances.
hainesr Oct 6, 2019
1a21f39
Add a test for restoring file permissions on extract.
hainesr Oct 6, 2019
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
5 changes: 5 additions & 0 deletions lib/zip/dos_time.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,11 @@ def dos_equals(other)
to_i / 2 == other.to_i / 2
end

# Create a DOSTime instance from a vanilla Time instance.
def self.from_time(time)
local(time.year, time.month, time.day, time.hour, time.min, time.sec)
end

def self.parse_binary_dos_format(binaryDosDate, binaryDosTime)
second = 2 * (0b11111 & binaryDosTime)
minute = (0b11111100000 & binaryDosTime) >> 5
Expand Down
16 changes: 10 additions & 6 deletions lib/zip/entry.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,24 +406,28 @@ def get_extra_attributes_from_path(path) # :nodoc:
@unix_uid = stat.uid
@unix_gid = stat.gid
@unix_perms = stat.mode & 0o7777
@time = ::Zip::DOSTime.from_time(stat.mtime)
Copy link
Member

@jdleesmiller jdleesmiller Oct 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

  • This is currently guarded by return if Zip::RUNNING_ON_WINDOWS. I am pretty sure that the modification time would also be available on Windows, and from what I can see in the documentation, File::stat should still work. I have been meaning to find a Windows machine to test this on... I guess even if the restore_times only works on *nix, it would still be an improvement.

  • What do you think to using time= rather than assigning the @time instance variable directly? It looks like that would also set some related attributes in @extra, which seems desirable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, I will try using time=. TBH working through how all this works was horribly convoluted so I must have just missed this subtlety 😬

Copy link
Member Author

Choose a reason for hiding this comment

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

Urgh. So I don't think the UniversalTime extra field can work as currently implemented. In Entry#time= when it creates a UniversalTime entry in @extra the .flag field is set to nil. But the UniversalTime field can only be packed if @flag is non-nil.

Going through the tests for the extra field code it seems that @flag is always set to 3 for UniversalTime, and indeed forcing this does work. But I need to look and see if this reasonable.

It's late now, so I'll pick this up another time.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right. I think given the issues with using time= at the moment, I think it's best to keep using @time for now.

I've found documentation for the UniversalTime extra field now, so I think I can fix it in a different PR once I've properly got my head round it.

I also think, now I've seen what adding the extra fields does to the archive, that having a way of not using extra fields by default (as suggested in #398) would be a good idea. So I'll have a think about that too.

I'll finish up making the other changes to this PR we've already discussed this evening and update it.

end

def set_unix_permissions_on_path(dest_path)
# BUG: does not update timestamps into account
def set_unix_attributes_on_path(dest_path)
# ignore setuid/setgid bits by default. honor if @restore_ownership
unix_perms_mask = 0o1777
unix_perms_mask = 0o7777 if @restore_ownership
::FileUtils.chmod(@unix_perms & unix_perms_mask, dest_path) if @restore_permissions && @unix_perms
::FileUtils.chown(@unix_uid, @unix_gid, dest_path) if @restore_ownership && @unix_uid && @unix_gid && ::Process.egid == 0
# File::utimes()

# Restore the timestamp on a file. This will either have come from the
# original source file that was copied into the archive, or from the
# creation date of the archive if there was no original source file.
::FileUtils.touch(dest_path, mtime: time) if @restore_times
Copy link
Member

Choose a reason for hiding this comment

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

Possibly one for a future PR... but we could use File::utime here as the original comment hinted, if we used the atime and ctime from the UniversalTime extra field. (And we could store those from File::stat, too.)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I used mtime because that is the only one FileUtils.touch supports. But now I look at the code for FileUtils.touch it turns out it uses File.utime internally anyway [1], setting atime and mtime to the same thing.

So, I agree, shall we leave this as is for now and look at it in a new PR?

[1] https://ruby-doc.org/stdlib-2.4.7/libdoc/fileutils/rdoc/FileUtils.html#method-c-touch

Copy link
Member

Choose a reason for hiding this comment

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

Good points. I am definitely OK with that 👍.

end

def set_extra_attributes_on_path(dest_path) # :nodoc:
return unless file? || directory?

case @fstype
when ::Zip::FSTYPE_UNIX
set_unix_permissions_on_path(dest_path)
set_unix_attributes_on_path(dest_path)
end
end

Expand Down Expand Up @@ -601,8 +605,6 @@ def create_file(dest_path, _continue_on_exists_proc = proc { Zip.continue_on_exi
end
::File.open(dest_path, 'wb') do |os|
get_input_stream do |is|
set_extra_attributes_on_path(dest_path)

bytes_written = 0
warned = false
buf = ''.dup
Expand All @@ -621,6 +623,8 @@ def create_file(dest_path, _continue_on_exists_proc = proc { Zip.continue_on_exi
end
end
end

set_extra_attributes_on_path(dest_path)
end

def create_directory(dest_path)
Expand Down
22 changes: 16 additions & 6 deletions lib/zip/file.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,21 +51,31 @@ class File < CentralDirectory
DATA_BUFFER_SIZE = 8192
IO_METHODS = [:tell, :seek, :read, :close]

DEFAULT_OPTIONS = {
restore_ownership: false,
restore_permissions: false,
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this used to default to true (and, unlike restore_times, did actually do something). Is there a particular reason to default it to false now?

Copy link
Member Author

Choose a reason for hiding this comment

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

It did default to true, but it didn't do anything! I forgot to mention this in the original PR.

In Entry#create_file the call to set the attributes of a file (set_extra_attributes_on_path) was being called too early - before the file was closed - so the permissions weren't being set correctly. In the course of my PR I moved the call to set_extra_attributes_on_path to when the file is closed to fix this. (Entry#create_directory got this right already.)

Then I set this to be be false by default for consistency. I certainly think that permissions should be restored by default though.

I'm happy to change this to true while I'm making the other changes required.

Copy link
Member

@jdleesmiller jdleesmiller Oct 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Aha, I didn't pick up on that. 👍 for false in that case. Edit: I think it makes sense to maintain compatibility initially but then in a future (major?) version bump change the defaults.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I will leave it set to false for now then.

restore_times: false
}.freeze

attr_reader :name

# default -> false
# default -> false.
attr_accessor :restore_ownership
# default -> false

# default -> false, but will be set to true in a future version.
attr_accessor :restore_permissions
# default -> true

# default -> false, but will be set to true in a future version.
attr_accessor :restore_times

# Returns the zip files comment, if it has one
attr_accessor :comment

# Opens a zip archive. Pass true as the second parameter to create
# a new archive if it doesn't exist already.
def initialize(path_or_io, create = false, buffer = false, options = {})
super()
options = DEFAULT_OPTIONS.merge(options)
@name = path_or_io.respond_to?(:path) ? path_or_io.path : path_or_io
@comment = ''
@create = create ? true : false # allow any truthy value to mean true
Expand Down Expand Up @@ -98,9 +108,9 @@ def initialize(path_or_io, create = false, buffer = false, options = {})

@stored_entries = @entry_set.dup
@stored_comment = @comment
@restore_ownership = options[:restore_ownership] || false
@restore_permissions = options[:restore_permissions] || true
@restore_times = options[:restore_times] || true
@restore_ownership = options[:restore_ownership]
@restore_permissions = options[:restore_permissions]
@restore_times = options[:restore_times]
end

class << self
Expand Down
89 changes: 89 additions & 0 deletions test/file_options_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
require 'test_helper'

class FileOptionsTest < MiniTest::Test
ZIPPATH = ::File.join(Dir.tmpdir, 'options.zip').freeze
TXTPATH = ::File.expand_path(::File.join('data', 'file1.txt'), __dir__).freeze
TXTPATH_600 = ::File.join(Dir.tmpdir, 'file1.600.txt').freeze
TXTPATH_755 = ::File.join(Dir.tmpdir, 'file1.755.txt').freeze
EXTPATH_1 = ::File.join(Dir.tmpdir, 'extracted_1.txt').freeze
EXTPATH_2 = ::File.join(Dir.tmpdir, 'extracted_2.txt').freeze
EXTPATH_3 = ::File.join(Dir.tmpdir, 'extracted_3.txt').freeze
ENTRY_1 = 'entry_1.txt'.freeze
ENTRY_2 = 'entry_2.txt'.freeze
ENTRY_3 = 'entry_3.txt'.freeze

def teardown
::File.unlink(ZIPPATH) if ::File.exist?(ZIPPATH)
::File.unlink(EXTPATH_1) if ::File.exist?(EXTPATH_1)
::File.unlink(EXTPATH_2) if ::File.exist?(EXTPATH_2)
::File.unlink(EXTPATH_3) if ::File.exist?(EXTPATH_3)
::File.unlink(TXTPATH_600) if ::File.exist?(TXTPATH_600)
::File.unlink(TXTPATH_755) if ::File.exist?(TXTPATH_755)
end

def test_restore_permissions
# Copy and set up files with different permissions.
::FileUtils.cp(TXTPATH, TXTPATH_600)
::File.chmod(0600, TXTPATH_600)
::FileUtils.cp(TXTPATH, TXTPATH_755)
::File.chmod(0755, TXTPATH_755)

::Zip::File.open(ZIPPATH, true) do |zip|
zip.add(ENTRY_1, TXTPATH)
zip.add(ENTRY_2, TXTPATH_600)
zip.add(ENTRY_3, TXTPATH_755)
end

::Zip::File.open(ZIPPATH, false, restore_permissions: true) do |zip|
zip.extract(ENTRY_1, EXTPATH_1)
zip.extract(ENTRY_2, EXTPATH_2)
zip.extract(ENTRY_3, EXTPATH_3)
end

assert_equal(::File.stat(TXTPATH).mode, ::File.stat(EXTPATH_1).mode)
assert_equal(::File.stat(TXTPATH_600).mode, ::File.stat(EXTPATH_2).mode)
assert_equal(::File.stat(TXTPATH_755).mode, ::File.stat(EXTPATH_3).mode)
end

def test_restore_times_true
::Zip::File.open(ZIPPATH, true) do |zip|
zip.add(ENTRY_1, TXTPATH)
zip.add_stored(ENTRY_2, TXTPATH)
end

::Zip::File.open(ZIPPATH, false, restore_times: true) do |zip|
zip.extract(ENTRY_1, EXTPATH_1)
zip.extract(ENTRY_2, EXTPATH_2)
end

assert_time_equal(::File.mtime(TXTPATH), ::File.mtime(EXTPATH_1))
assert_time_equal(::File.mtime(TXTPATH), ::File.mtime(EXTPATH_2))
end

def test_restore_times_false
::Zip::File.open(ZIPPATH, true) do |zip|
zip.add(ENTRY_1, TXTPATH)
zip.add_stored(ENTRY_2, TXTPATH)
end

::Zip::File.open(ZIPPATH, false, restore_times: false) do |zip|
zip.extract(ENTRY_1, EXTPATH_1)
zip.extract(ENTRY_2, EXTPATH_2)
end

assert_time_equal(::Time.now, ::File.mtime(EXTPATH_1))
assert_time_equal(::Time.now, ::File.mtime(EXTPATH_2))
end

private

# Method to compare file times. DOS times only have 2 second accuracy.
def assert_time_equal(expected, actual)
assert_equal(expected.year, actual.year)
assert_equal(expected.month, actual.month)
assert_equal(expected.day, actual.day)
assert_equal(expected.hour, actual.hour)
assert_equal(expected.min, actual.min)
assert_in_delta(expected.sec, actual.sec, 1)
end
end