fix: change directory attributes detection#851
Conversation
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
This PR modifies the directory attributes detection logic in src/types.rs. While the reorganization of constants and improved documentation are good, there is a critical logic error in the MS-DOS symlink detection that needs to be addressed.
Critical Issue
The new symlink check in the System::Dos branch incorrectly interprets the high 16 bits of external_attributes as Unix mode. According to the ZIP specification, for DOS-system archives, only the low 16 bits are meaningful (MS-DOS attributes), and the high 16 bits should not be used for Unix-style mode detection. This could cause incorrect symlink identification.
Recommendations
Before merging, please:
- Review the symlink detection logic for DOS system archives
- Verify the behavior with test cases for DOS archives with and without the symlink bits accidentally set in the high 16 bits
- Ensure the changes align with APPNOTE.TXT section 4.4.15 regarding DOS compatibility
The PR is marked as WIP, so please address this issue before requesting final review.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
| } else if (unix_mode & ffi::S_IFMT) == ffi::S_IFLNK { | ||
| ffi::S_IFLNK | 0o0777 |
There was a problem hiding this comment.
🛑 Logic Error: The symlink check using unix_mode & ffi::S_IFMT in the System::Dos branch is incorrect. For DOS system archives, the external_attributes follow the MS-DOS specification where only the low 16 bits are meaningful (DOS attributes). The high 16 bits should not be interpreted as Unix mode for DOS systems. This check could incorrectly identify files as symlinks when the high 16 bits happen to match the symlink pattern due to uninitialized or arbitrary data. According to the ZIP specification (APPNOTE.TXT section 4.4.15), for DOS compatibility, only the low byte contains the MS-DOS directory attribute byte.
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
1 participant
WIP for #737