8336498: [macos] [build]: install-file macro may run into permission denied error

[RealLucy]

On MacOS, files may have extended attributes attached. These attributes are copied together with the files. To prevent issues during further processing, the extended attributes of the copies must be removed. This action was implemented as solution of an older bug.

The solution is incomplete because it does not handle files with read-only permissions correctly. Without write permission, matter cannot remove the extended attributes. The action is rejected with a "permission denied" error.

The issue is present in all releases. I reproduced it in 11, 17, ... 23, head

The solution is to "chmod u+w" only those files which need to have their extended attributes removed.

Backport note: in releases prior to jdk23, the change needs to go into file MakeBase.gmk.

Testing @SAP completed without any related issues.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8336498: [macos] [build]: install-file macro may run into permission denied error (Bug - P4)

Reviewers

• Christoph Langer (@RealCLanger - Reviewer)
• Erik Joelsson (@erikj79 - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/20203/head:pull/20203
$ git checkout pull/20203

Update a local copy of the PR:
$ git checkout pull/20203
$ git pull https://git.openjdk.org/jdk.git pull/20203/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 20203

View PR using the GUI difftool:
$ git pr show -t 20203

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/20203.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back lucy! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@RealLucy This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8336498: [macos] [build]: install-file macro may run into permission denied error

Reviewed-by: clanger, erikj

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 275 new commits pushed to the master branch:

• 358d77d: 8337657: AArch64: No need for acquire fence in safepoint poll during JNI calls
• 60fa08f: 8337797: Additional ExternalAddress cleanup
• 3cf3f30: 8330191: Fix typo in precompiled.hpp
• 069e0ea: 8338064: Give better error for ConcurrentHashTable corruption
• 2b5aec2: 8338109: java/awt/Mouse/EnterExitEvents/ResizingFrameTest.java duplicate in ProblemList
• 1407160: 8313931: Javadoc: links to type parameters actually generate links to classes
• c37e863: 8314125: RISC-V: implement Base64 intrinsic - encoding
• 6ebd5d7: 8338036: Serial: Remove Generation::update_counters
• 53fce38: 8338062: JFR: Remove TestStartDuration.java and TestStartName.java from ProblemList.txt
• f74109b: 8337939: ZGC: Make assertions and checks less convoluted and explicit
• ... and 265 more: https://git.openjdk.org/jdk/compare/a60608e7a35aeeed57bcce641d4957de1e4b4def...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[openjdk]

@RealLucy The following label will be automatically applied to this pull request:

• build

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 00: Full (8e9cb6fe)

[RealCLanger]

Looks trivial. Can you hint on how to reproduce the issue? E.g. for which file I should remove the user write permission to get the build failing?

[RealLucy]

Thank you for the review, @RealCLanger !

Creating a reproducer is not completely trivial. There are two file properties that must be true at the same time:

1. The destination file must be read-only after copying
2. The destination file must have extended attributes attached.

You can prepare any source file accordingly (file mode and attributes are copied with the command parameters used). In my setup, make/langtools/tools/propertiesparser/resources/templates.properties has an extended attribute, but r/w permissions. If I change those to r/o, the build will fail without the fix.

According to my experience, it is unpredictable which files will satisfy the above requirements. On good days, no file has attributes attached while being r/o. On bad days, there are multiple such files.

[jaikiran]

Hello @RealLucy, On a general note, after removing the extended attributes, should we consider switching the permissions of that file back to the original?

[RealLucy]

Hello @RealLucy, On a general note, after removing the extended attributes, should we consider switching the permissions of that file back to the original?

Yes, I had that thought. But what was the original state? How could we remember it? Was the file r/w already before? I did not want to over-engineer the fix, so I kept it as simple as possible.

[RealLucy]

Last chance to comment. Otherwise, I will integrate during the weekend.

[jaikiran]

Hello Lutz, would it be possible to wait a few more days on this one? It would be good to have someone from the build area to review this. They are out currently. In the meantime, I will run this change in our CI to verify nothing breaks.

[RealLucy]

Hello Lutz, would it be possible to wait a few more days on this one? ...

Sure, I'll stand by.

[jaikiran]

Hello Lutz,

In the meantime, I will run this change in our CI to verify nothing breaks.

I ran tier1 through tier8 with this change, in our CI and I didn't see anything related fail.

[RealLucy]

Any new opinion on this PR? From the build people, for example?

[magicus]

Hi, I'm back from vacation now.

I feel a bit okay-ish about this fix. To be honest, we have not really been good at ensuring proper file permissions at all times, but instead resorted to this kind of adhoc-fixes whenever problems arise. So in that spirit, I guess this is as good as any of the previous such fixes.

But on the other hand, I don't like that state of affairs. I'm even more vexed by the fact that you state that the extended attributes and/or the write protection come and go erratically. I think it might be wise to spend a bit more effort trying to figure out why this happens.

[RealLucy]

Hi, I'm back from vacation now.

Looks like we are taking turns. Now I'm on vacation. Thorough analysis of why the issue pops up occasionally will have to wait until I'm back (Aug 19th). I have no specific idea in which direction I should start my search, though. It might be an influence from the environment.

[magicus]

Enjoy your vacation! 🌴

I'd suggest starting by checking all files for extended attributes and their permissions (like ls -lr > perms.txt on the build directory, save this file and compare it from run to run).

[erikj79]

If we want to make a file read-only in the build output, it needs to happen explicitly after any copy operation. I think it's fine to leave write permission for the user in this case.

The only reason I could imagine this happening is if the source tree (or subsets thereof) has somehow ended up read-only. That seems like a user environment problem, but we shouldn't fail like this because of it.

[RealLucy]

Back from vacation, working through the pile of tasks...

@erikj79 An influence of the user environment, that is my suspicion as well (see above).

I would go ahead an integrate tomorrow (Wednesday), if there is no veto.

[erikj79]

I'm still ok with this patch.

[RealLucy]

/integrate

[openjdk]

Going to push as commit 715fa8f.
Since your change was applied there have been 383 commits pushed to the master branch:

• e88a3b0: 8338661: StackMapTable is invalid if frames appear in dead code
• 5981697: 8337828: CDS: Trim down minimum GC region alignment
• cafb3dc: 6318027: BasicScrollBarUI does not disable timer when enclosing frame is disabled.
• 88ccbb6: 8336934: Clean up JavaLangReflectAccess
• d728107: 8338482: com/sun/jdi/ThreadMemoryLeakTest.java requires that compressed oops are enabled
• 1ebf2cf: 8336756: Improve ClassFile Annotation writing
• 0267284: 8338611: java.lang.module specification wording not aligned with JEP 261
• c646efc: 8205957: setfldw001/TestDescription.java fails with bad field value
• 285ceb9: 8336529: (fs) UnixFileAttributeViews setTimes() failing on armhf, Ubuntu noble
• 55a97ec: 8336729: C2: Div/Mod nodes without zero check could be split through iv phi of outer loop of long counted loop nest resulting in SIGFPE
• ... and 373 more: https://git.openjdk.org/jdk/compare/a60608e7a35aeeed57bcce641d4957de1e4b4def...master

Your commit was automatically rebased without conflicts.

[openjdk]

@RealLucy Pushed as commit 715fa8f.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.