Skip to content

Fix loading extra fields #459

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.

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 7 commits into from
Feb 14, 2021
Merged

Fix loading extra fields #459

merged 7 commits into from
Feb 14, 2021

Conversation

hainesr
Copy link
Member

@hainesr hainesr commented Oct 3, 2020

This PR fixes a fairly serious bug when reading in extra fields.

Previously, only the extra fields stored in the central directory were being read in. In reality it is often the case that the extra field in the central directory is just a marker, and the full data is in the local header. So we need to read both in and merge the two into the final correct extra field. This merging infrastructure was already implemented in the extra field code but we never actually read the local extra fields in until now.

Reading the central directory headers and local entry headers seems rather fragile, so we can't just read one over the other and hope to end up with a correctly merged set of extra fields because this breaks other things. So we need to specifically read the local extra field data and merge just those bits.

As previously implemented the `uid` and `gid` fields could only ever be
read as 0, because they were being initialized to zero and then
memoization (`@uid ||= uid`) was used to 'save' the new value. Using `nil`
as the initial value for either of these fields breaks so many tests, so I
have fixed this by not using memoization instead. This is safe because it
is only the local extra field that holds these values for this type of
extra field.
Remove some (almost) duplicated code and get ready for the real fix.
Previously, only the extra fields stored in the central directory were
being read in. In reality it is often the case that the extra field in
the central directory is just a marker, and the full data is in the
local header. So we need to read both in and merge the two into the
final correct extra field. This merging infrastructure was already
implemented in the extra field code but we never actually read the
local extra fields in until now.

Reading the central directory headers and local entry headers seems
rather fragile, so we can't just read one over the other and hope to end
up with a correctly merged set of extra fields because this breaks other
things. So we need to specifically read the local extra field data and
merge just those bits.

This commit also fixes a couple of tests that were 'broken' by us now
reading extra fields in correctly!
From what I can tell this was erroneously copied out of extra_field.rb
during a refactor. It attempts to compare a non-existent Hash that used
to be inherited before the refactor. If this code had been left within
ExtraField it would make more sense, but as it's not needed there either
let's just remove it.

See 20d79fe for the refactor.
@coveralls
Copy link

Coverage Status

Coverage increased (+1.0%) to 96.72% when pulling 0235e76 on hainesr:fix-extra-fields into e5e3f97 on rubyzip:master.

@simonoff simonoff merged commit 410daad into rubyzip:master Feb 14, 2021
@hainesr hainesr deleted the fix-extra-fields branch February 14, 2021 14:59
@hainesr hainesr mentioned this pull request May 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants