Skip to content

Fix Zip rcount field width in x0015 and x0016 certificate id extra fields#787

Merged
garydgregory merged 1 commit into
apache:masterfrom
kali834x:x0015-x0016-rcount-field-width
Jul 17, 2026
Merged

Fix Zip rcount field width in x0015 and x0016 certificate id extra fields#787
garydgregory merged 1 commit into
apache:masterfrom
kali834x:x0015-x0016-rcount-field-width

Conversation

@kali834x

Copy link
Copy Markdown
Contributor

the certificate-id extra fields read rcount as a 2-byte zipshort at offset and read hashalg from offset+2, but the field layout documented on both classes (and the reading in the sibling x0017_strongencryptionheader) puts a 4-byte rcount first with hashalg at offset+4. as written, x0015_certificateidforfile.parseFromCentralDirectoryData and the x0016 equivalent expose only the low 16 bits of rcount through getRecordCount and derive getHashAlgorithm from the high half of the count. read rcount as a 4-byte ziplong and hashalg at offset+4 in both, and raise the minimal-length check from 4 to 6 so a block too short to hold both fields is rejected with a zipexception rather than parsed into garbage. getRecordCount stays int (the 4 bytes are read then narrowed) so the public api is unchanged.

  • Read the contribution guidelines for this project.
  • Read the ASF Generative Tooling Guidance if you use Artificial Intelligence (AI).
  • I used AI to create any part of, or all of, this pull request. Which AI tool was used to create this pull request, and to what extent did it contribute?
  • Run a successful build using the default Maven goal with mvn; that's mvn on the command line by itself.
  • Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied. This may not always be possible, but it is a best practice.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Each commit in the pull request should have a meaningful subject line and body. Note that a maintainer may squash commits during the merge process.

x0015_certificateidforfile and x0016_certificateidforcentraldirectory read the certificate-id extra field's rcount as a 2-byte zipshort at offset and hashalg at offset+2, but the layout documented on both classes (and the reading in the sibling x0017_strongencryptionheader) puts a 4-byte rcount first with hashalg at offset+4. getrecordcount therefore exposed only the low 16 bits of rcount and gethashalgorithm was derived from its high half. read rcount as a ziplong and hashalg at offset+4 in both, and raise the minimal length from 4 to 6 so a block too short to hold both fields is rejected with a zipexception instead of parsed into garbage.
@garydgregory garydgregory changed the title fix rcount field width in x0015 and x0016 certificate id extra fields Fix rcount field width in x0015 and x0016 certificate id extra fields Jul 17, 2026
@garydgregory
garydgregory merged commit 18e0264 into apache:master Jul 17, 2026
19 checks passed
@garydgregory

Copy link
Copy Markdown
Member

Thank you @kali834x , merged 🚀 Would you please check if we have other Zip Short/Long mismatches?

@garydgregory garydgregory changed the title Fix rcount field width in x0015 and x0016 certificate id extra fields Fix Zip rcount field width in x0015 and x0016 certificate id extra fields Jul 17, 2026
@kali834x

Copy link
Copy Markdown
Contributor Author

Thanks for merging. I went through the rest of the extra field parsers in the zip package (x0014, x0017, x0019, x000a ntfs, x5455, x7875, zip64, 0xa11e resource alignment, ASi, unicode path/comment) against their documented layouts and didn't find any other Short/Long width mismatches. x0017 already reads rcount as 4 bytes in both the central directory and local file paths, and the others either have no fixed numeric fields or read exactly the widths their specs give. The only thing I noticed is the offset arithmetic in X0017.parseFileFormat that the existing TODO comments already question, but that's about offset constants rather than field widths.

@garydgregory

Copy link
Copy Markdown
Member

@kali834x
Thanks for checking.

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.

2 participants