RFR: 8321274: Rename ZipEntry.extraAttributes to ZipEntry.externalFileAttributes [v4]

Eirik Bjørsnøs eirbjo at openjdk.org
Sat Jun 29 18:24:46 UTC 2024


On Sun, 12 May 2024 14:37:23 GMT, Chen Liang <liach at openjdk.org> wrote:

> I believe this field only captures the 2 bytes at higher indices of External File Attribute; it's not the complete 4-byte external file attributes and this value is not captured unless "Version made by" field's larger index byte (byte 5) is 3. So this name may still be imperfect.

The purpose of this PR was mainly to reduce the risk of confusion with the "extra field", which is an entirely unrelated concept.

While I agree that the name `externalFileAttributes` might not express the full semantics of the field perfectly, I'm honestly not sure which name would, given that `APPNOTE.TXT` is pretty opaque in describing what these two bytes express, and as you point out, the field only carries data when "Version Made By" indicates Unix.

Instead of trying to find name which covers the full semantics, I suggest that we keep `externalFileAttributes` and instead seek to improve the comments relevant to this field:

* The field comment currently says `// File type, setuid, setgid, sticky, POSIX permissions`, we could prepend something clarifying that the field only carries data for Unix entries.
* Line 699 says `// read all bits in this field, including sym link attributes`, this could be improved to clarify that only the higher two "unix-bytes" are read ("all bits in this field" is a bit unclear now)
* Line 700 masks off the Unix bytes with `0xFFFF`, this isn't strictly necessary since `CENATX_PERMS` already only reads the two bytes we want. Perhaps clearer just to drop this masking.
* `ZipUtils.CENATX_PERMS` reads the full 16 bits of the Unix external file attributes, not just the permissions. The current name is confusing.

Since I don't want to delay the integration of the this PR any further, I suggest we tackle the above clarifications in a follow-up PR. Would that be ok with you, @liach ?

-------------

PR Comment: https://git.openjdk.org/jdk/pull/16952#issuecomment-2198283812



More information about the security-dev mailing list