Fix permissions on new zip files (#294) #300

hainesr · 2016-09-01T11:40:45Z

This is an attempt to fix #294 in a more sensible way than was attempted, by me :-), previously.

I used the improved tests by @metavida.

To fix it I stopped the original temporary files from being created with 0600 permissions. Is this sensible? Have I missed something?

...instead of comparing against in-ruby subtraction math.

This test fails because in *NIX when the umask is 0027, files are created with 0640 permissions. The current implementation of Zip::File.create_file_permissions does a simple subtraction of 0666 - 0027 == 0637

Before this change: 1) Failure: FilePermissionsTest#test_umask_027 [/Users/marcoswk/workspace/rubyzip/test/file_permissions_test.rb:52]: Expected: 33184 Actual: 33183 After this change: 1) Failure: FilePermissionsTest#test_umask_027 [/Users/marcoswk/workspace/rubyzip/test/file_permissions_test.rb:52]: --- expected +++ actual @@ -1,2 +1,2 @@ # encoding: US-ASCII -"100640" +"100637"

This fixes rubyzip#294 in what I hope is a more sensible way than trying to mess with umasks, etc, directly. Temporary files were being created with 0600 permissions and then being set to different permissions, based on umask, etc, afterwards. I don't know what the rationale for this was, but there were errors in the umask calculations when moving from the temporary file to the intended end result.

Now we don't differentiate between Windows and Linux in the library code for this, we don't need separate tests.

coveralls · 2016-09-01T11:47:00Z

Coverage increased (+0.3%) to 94.295% when pulling b005c48 on hainesr:fix-create-perms into 7aa3666 on rubyzip:master.

metavida · 2016-09-05T00:17:10Z

lib/zip/file.rb

      if yield tmp_filename
        ::File.rename(tmp_filename, name)
-        ::File.chmod(@file_permissions, name) if defined?(@file_permissions)
+        ::File.chmod(@file_permissions, name) if @create.nil?


Why change this to if @create.nil? instead of defined?(@file_permissions)

With your removal of the @file_permissions setter in the when create branch should mean that that variable is only-ever set for the non-create path.

I also lean towards avoiding a .nil? check on the @create variable, because the desired values for @create are a bit fuzzy to me, as a developer who's a bit unfamiliar with the code. The code comment for the initialize method suggests that @create should be either nil or true, but the constant is CREATE = 1 😃

The main reasons I stopped using the @file_permissions variable here are:

The logic for whether we need to change the permissions here is based upon whether the file is created or not, not whether there exists some file permissions. So,

If someone decided to use the @file_permissions variable for something else, we might get an odd bug here.

I agree with your concerns about the @create variable though. I'll look at whether I can make its use consistent.

metavida · 2016-09-05T00:34:28Z

@hainesr, looks sensible to me! There's one note suggesting not using a @create.nil? check but that's up to you. You've got my 👍 to merge this! Thanks for the fix and for cleaning up my unit tests too!

hainesr · 2016-09-05T09:07:58Z

Thanks for your notes @metavida! I'll see if I can fix the usage of the @create variable in a sensible way to make that bit a little more consistent. I definitely agree with your concerns with it.

metavida · 2016-09-05T12:28:35Z

Maybe something as simple as switching to a private method like this would do the trick:

def create?
  # In order to not break backwards comparability
  # allow any truthy value to work. 
  @create
end

Then updating the code comment for initislize to reference the CREATE constant, just so that it matches the rest of the code samples.

hainesr · 2016-09-05T12:45:47Z

I've been playing with this today. I added a couple of debug puts to see what values @create is ever set to and got some very odd results!

There were many cases where @create was set to {:mode=>193}! It took me ages to find out why this was but I eventually tracked it down to here: https://github.com/rubyzip/rubyzip/blob/master/lib/zip/file.rb#L419

f = File.open(tmpname, opts)

This, of course, calls the Zip::File.open method, passing {:mode=>193} into the @create variable, but I think it's supposed to be calling ::File.open. Changing that line to

f = ::File.open(tmpname, opts)

still passes all the tests and I think will make it easier to fix @create to be more consistent.

I'll add a commit to this pull request soon.

hainesr · 2016-09-05T13:03:24Z

Hmm, actually, as that file is simply opened and then immediately closed, maybe the only reason that code is there was to set the permissions and raise an error if the file already existed?

Given we don't want the permissions to be set any more, and the file will not exist as we're generating a new random filename, then maybe it could be refactored out completely.

I'll have a go and see what happens.

Combine the creation of the temporary filename with the writing to it.

@create

Make the internal @create varible more consistent and actually match the documentation. Zip::File::CREATE is now true, rather than 1. A new test is added to check if passing 1 in still works to ensure backwards compatibility.

hainesr · 2016-09-05T14:26:39Z

Right, this pull request has ended up fixing a few other internal bits which were broken/inconsistent.

Comments appreciated.

coveralls · 2016-09-05T14:27:53Z