[8.6.0] Force rctx.{download_and,}extract to create user-readable files (ht…#28551
[8.6.0] Force rctx.{download_and,}extract to create user-readable files (ht…#28551iancha1992 merged 1 commit intobazelbuild:release-8.6.0from
rctx.{download_and,}extract to create user-readable files (ht…#28551Conversation
…zelbuild#28531) Archives in the wild do sometimes contain non-readable files, but other tools work around this and thus mask their brokenness. Context: https://bazelbuild.slack.com/archives/CDCMRLS23/p1770213515354229 Closes bazelbuild#28531. PiperOrigin-RevId: 865960367 Change-Id: I7273eb983d63d6960d184764cec5040bba77b2c2
iancha1992
added
the
team-ExternalDeps
iancha1992
added
the
awaiting-review
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses an issue where files within archives might have non-readable permissions, causing problems after extraction. The change forces all extracted files from .ar, .tar.*, and .zip archives to have at least user-read permissions by adding the 0400 mode bit. This is a sensible workaround for malformed archives and improves Bazel's robustness. The implementation is straightforward and consistently applied across the different decompressor functions. The accompanying shell tests are thorough, covering all three archive types with specifically crafted non-readable files to verify the fix. The changes look correct and well-tested. I have a few suggestions to improve maintainability by replacing the magic number for permissions with a named constant.
| filePath.chmod(entry.getMode()); | ||
| // Ensure that all files are at least user-readable. Some archives contain files that | ||
| // are not, but many other tools are working around this and thus mask these issues. | ||
| filePath.chmod(entry.getMode() | 0400); |
There was a problem hiding this comment.
Using the magic number 0400 for file permissions can make the code harder to understand and maintain. It's better to define a named constant to represent this value, for example: private static final int USER_READ_PERMISSION = 0400;. This makes the intent of the code clearer and reduces the risk of typos when dealing with security-sensitive permission bits.
| filePath.chmod(entry.getMode()); | ||
| // Ensure that all files are at least user-readable. Some archives contain files that | ||
| // are not, but many other tools are working around this and thus mask these issues. | ||
| filePath.chmod(entry.getMode() | 0400); |
There was a problem hiding this comment.
Using the magic number 0400 for file permissions can make the code harder to understand and maintain. It's better to define a named constant to represent this value, for example: private static final int USER_READ_PERMISSION = 0400;. This makes the intent of the code clearer and reduces the risk of typos when dealing with security-sensitive permission bits.
| outputPath.chmod(permissions); | ||
| // Ensure that all files are at least user-readable. Some archives contain files that | ||
| // are not, but many other tools are working around this and thus mask these issues. | ||
| outputPath.chmod(permissions | 0400); |
There was a problem hiding this comment.
This file already defines several constants for file permissions (e.g., S_IFDIR, S_IFREG, EXECUTABLE_MASK). For consistency, readability, and safety, the magic number 0400 should also be defined as a named constant, for example: private static final int USER_READ_PERMISSION = 0400;.
Pull request was closed
github-actions
bot
removed
the
awaiting-review
github-actions
bot
added
the
awaiting-review
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
c2fa571
github-actions
bot
removed
the
awaiting-review
### What does this PR do? Bump `.bazelversion` from 8.5.1 to 8.6.0. ### Motivation Selected changes between 8.5.1 and 8.6.0: - Fix visibility for implicit deps of parent rules (bazelbuild/bazel#28722) - Force rctx.{download_and,}extract to create user-readable files (bazelbuild/bazel#28551) - Fix disk cache failures on concurrent read-write access on Windows (bazelbuild/bazel#28529) - Add a target_type argument to ctx.actions.symlink (bazelbuild/bazel#28538) - Compensate for Windows filesystems lacking junction support (bazelbuild/bazel#28367) (our fix) - Add short_uncached and detailed_uncached options to --test_summary (bazelbuild/bazel#28343) - Add --experimental_strict_repo_env option (bazelbuild/bazel#28189) - Make overlaid files executable in http_archive (bazelbuild/bazel#28277) - Add bazel mod show_repo --all_repos and --all_visible_repos (bazelbuild/bazel#28012) - Enable --experimental_retain_test_configuration_across_testonly (bazelbuild/bazel#28115) - Add option to continue with local execution if remote cache is unavailable (bazelbuild/bazel#28001)
### What does this PR do? Bump `.bazelversion` from 8.5.1 to 8.6.0. ### Motivation Selected changes between 8.5.1 and 8.6.0: - Fix visibility for implicit deps of parent rules (bazelbuild/bazel#28722) - Force rctx.{download_and,}extract to create user-readable files (bazelbuild/bazel#28551) - Fix disk cache failures on concurrent read-write access on Windows (bazelbuild/bazel#28529) - Add a target_type argument to ctx.actions.symlink (bazelbuild/bazel#28538) - Compensate for Windows filesystems lacking junction support (bazelbuild/bazel#28367) (our fix) - Add short_uncached and detailed_uncached options to --test_summary (bazelbuild/bazel#28343) - Add --experimental_strict_repo_env option (bazelbuild/bazel#28189) - Make overlaid files executable in http_archive (bazelbuild/bazel#28277) - Add bazel mod show_repo --all_repos and --all_visible_repos (bazelbuild/bazel#28012) - Enable --experimental_retain_test_configuration_across_testonly (bazelbuild/bazel#28115) - Add option to continue with local execution if remote cache is unavailable (bazelbuild/bazel#28001)
### What does this PR do? Bump `bazel` version from 8.5.1 to 8.6.0 to benefit from a series of improvements and fixes. Ours (bazelbuild/bazel#28367) allows to re-enable "convenience symlinks" for Windows users and makes [`path.realpath`](https://bazel.build/rules/lib/builtins/path#realpath) succeed when sharing a folder between a Linux host and a Windows VM. ### Motivation Selected changes between 8.5.1 and 8.6.0: - 💡 bazelbuild/bazel#28001 - bazelbuild/bazel#28012 - 💡 bazelbuild/bazel#28189 - bazelbuild/bazel#28277 - bazelbuild/bazel#28343 - 🐕 bazelbuild/bazel#28367 - bazelbuild/bazel#28529 - bazelbuild/bazel#28538 - bazelbuild/bazel#28551 - bazelbuild/bazel#28722 Co-authored-by: regis.desgroppes <[email protected]>
3 participants
…tps://github.com//pull/28531)
Archives in the wild do sometimes contain non-readable files, but other tools work around this and thus mask their brokenness.
Context: https://bazelbuild.slack.com/archives/CDCMRLS23/p1770213515354229
Closes #28531.
PiperOrigin-RevId: 865960367
Change-Id: I7273eb983d63d6960d184764cec5040bba77b2c2
Commit 0bb7836