8343020: (fs) Add support for SecureDirectoryStream on macOS #21696

dmlloyd · 2024-10-24T21:54:55Z

OpenJDK will not produce SecureDirectoryStreams on MacOS. Support for SecureDirectoryStream on UNIX-like OSes is predicated on the SUPPORTS_OPENAT flag in UnixNativeDispatcher. That flag in turn is set when the runtime environment supports openat, fstatat, unlinkat, renameat, futimesat, and fdopendir.

This fails on MacOS because futimesat does not exist on that platform, apparently having been a proposed-but-not-accepted part of POSIX some time ago. While there is an indirect replacement that is supported on MacOS - utimensat - this is not actually needed, because the unique functionality provided by futimesat (that is, performing the action of futimes relative to an open directory file descriptor) is not utilized, since the only place this function is used passes NULL as the relative filename argument.

Replacing this with simply calling futimes instead allows SecureDirectoryStream to function on MacOS.

Additionally, we must ensure that openat, fstatat, and fdopendir are properly detected on MacOS x64, because there are 32- and 64-bit variations on that platform which misbehave subtly when done improperly.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)

Issue

JDK-8343020: (fs) Add support for SecureDirectoryStream on macOS (New Feature - P4)

Reviewers

Brian Burkhalter (@bplb - Reviewer)
Alan Bateman (@AlanBateman - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21696

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

Using diff file

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

Webrev

Link to Webrev Comment

OpenJDK will not produce SecureDirectoryStreams on MacOS. Support for SecureDirectoryStream on UNIX-like OSes is predicated on the `SUPPORTS_OPENAT` flag in UnixNativeDispatcher. That flag in turn is set when the runtime environment supports `openat`, `fstatat`, `unlinkat`, `renameat`, `futimesat`, and `fdopendir`. This fails on MacOS because `futimesat` does not exist on that platform, apparently having been a proposed-but-not-accepted part of POSIX some time ago. While there is an indirect replacement that is supported on MacOS - `utimensat` - this is not actually needed, because the unique functionality provided by `futimesat` (that is, performing the action of `futimes` relative to an open directory file descriptor) is not utilized, since the only place this function is used passes `NULL` as the relative filename argument. Replacing this with simply calling `futimes` instead allows `SecureDirectoryStream` to function on MacOS.

bridgekeeper · 2024-10-24T21:55:39Z

👋 Welcome back dmlloyd! 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 · 2024-10-24T22:30:56Z

@dmlloyd 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:

8343020: (fs) Add support for SecureDirectoryStream on macOS

Reviewed-by: bpb, alanb

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 32 new commits pushed to the master branch:

1341b81: 8341666: FileInputStream doesn't support readAllBytes() or readNBytes(int) on pseudo devices
52382e2: 8338021: Support new unsigned and saturating vector operators in VectorAPI
e659d9d: 8342975: C2: Micro-optimize PhaseIdealLoop::Dominators()
9f6211b: 8341371: CDS cannot load archived heap objects with -XX:+UseSerialGC -XX:-UseCompressedOops
120a935: 8342561: Metaspace for generated reflection classes is no longer needed
d5fb6b4: 8339939: [JVMCI] Don't compress abstract and interface Klasses
a5ad974: 8343056: C2: Micro-optimize Node lists grow
ec06187: 8034066: Incorrect alignment in the "Code" section for "-c -XDdetails" options
eb3669a: 8340796: Use a consistent order when loading cxq and EntryList
0e3fc93: 8342083: Make a few fields in FileSystemPreferences final
... and 22 more: https://git.openjdk.org/jdk/compare/d1540e2a49c7a41eb771fc9896c367187d070dec...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@bplb, @AlanBateman) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

openjdk · 2024-10-24T23:12:41Z

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

nio

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 · 2024-10-24T23:15:16Z

Webrevs

bplb · 2024-10-24T23:49:27Z

There is no noreg-* label on the issue. Is this change covered by an existing test?

bplb · 2024-10-25T00:59:11Z

I'm seeing a build failure on macosx-x64 on both my own machine and in the CI:

Error occurred during initialization of boot layer
java.lang.InternalError: java.base not found

The platforms linux-{aarch64,x64} and macosx-aarch64 did not have this problem.

AlanBateman · 2024-10-25T08:17:04Z

There is no noreg-* label on the issue. Is this change covered by an existing test?

DirectoryStream/SecureDS.java is the test for this API. Right now it only runs on Linux (it used to run on Solaris too). If support is extended to macOS then we'll need to change this test to use @requires and drop the existing check in the test main.

AlanBateman · 2024-10-25T09:00:11Z

I'm seeing a build failure on macosx-x64 on both my own machine and in the CI:
Error occurred during initialization of boot layer
java.lang.InternalError: java.base not found

Can you see if -Xlog:init=debug to see if it reveals anything?

I have a vague memory of issues futimes not being supported on some versions of macOS or Linux and we had to use futimesat instead. Switching to futimes means testing on a wide range of OS releases to make sure that aren't any issues.

AlanBateman · 2024-10-25T09:16:38Z

Can you see if -Xlog:init=debug to see if it reveals anything?

I was able to duplicate this, it does break the macosx-x64 build, I think it means tracking down which macOS versions have futimes.

dmlloyd · 2024-10-25T11:20:42Z

There is no noreg-* label on the issue. Is this change covered by an existing test?

DirectoryStream/SecureDS.java is the test for this API. Right now it only runs on Linux (it used to run on Solaris too). If support is extended to macOS then we'll need to change this test to use @requires and drop the existing check in the test main.

OK I'll see if I can figure out how to do this.

I'm seeing a build failure on macosx-x64 on both my own machine and in the CI:
Error occurred during initialization of boot layer
java.lang.InternalError: java.base not found
Can you see if -Xlog:init=debug to see if it reveals anything?

I have a vague memory of issues futimes not being supported on some versions of macOS or Linux and we had to use futimesat instead. Switching to futimes means testing on a wide range of OS releases to make sure that aren't any issues.

Luckily I still have a x64 mac so I can test this locally too. I think futimesat is only supported on Linux and Solaris, at least according to https://www.gnu.org/software/gnulib/manual/html_node/futimesat.html if that can be trusted. So I think what's been happening is that my_futimesat has been null on most platforms, meaning that AFAICT, SDS is simply not available outside of Linux (and Solaris, I guess).

I can also try and see if utimensat works better, or even support futimesat if present and otherwise fallback to futimes and see how that goes.

dmlloyd · 2024-10-25T12:36:54Z

The debug output is not revealing much (the boot module finder just returns null for java.base), and it seems to be failing too early to attach a debugger, so I'm on to println debugging now.

Another logical discrepancy is that futimes does in fact seem to work in my standalone test.c testing. And anyway, why does boot depend on futimes? So I think the problem may be unrelated to futimes per se, but rather is due to another capability that is used by SecureDirectoryStream that is indirectly enabled. So I'm focusing my high tech debugging strategy in that area as well.

dmlloyd · 2024-10-25T12:45:24Z

There's an unexpected plot twist. It turns out there's some kind of off-by-one problem when reading the directory when SDS is active. So the exceptions being thrown are like:

java.nio.file.NoSuchFileException: /Volumes/Case-sensitive/Users/david/src/java/openjdk/build/macosx-x86_64-server-release/jdk/modules/ava.management.rmi
	at java.base/sun.nio.fs.UnixException.translateToIOException(UnixException.java:92)
	at java.base/sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:106)
	at java.base/sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:113)
	at java.base/sun.nio.fs.UnixFileAttributeViews$Basic.readAttributes(UnixFileAttributeViews.java:56)
	at java.base/sun.nio.fs.UnixFileSystemProvider.readAttributes(UnixFileSystemProvider.java:163)
	at java.base/java.nio.file.Files.readAttributes(Files.java:1865)
	at java.base/jdk.internal.module.ModulePath.scanDirectory(ModulePath.java:279)
	at java.base/jdk.internal.module.ModulePath.scan(ModulePath.java:233)
	at java.base/jdk.internal.module.ModulePath.scanNextEntry(ModulePath.java:191)
	at java.base/jdk.internal.module.ModulePath.find(ModulePath.java:155)
	at java.base/jdk.internal.module.SystemModuleFinders$1.lambda$find$0(SystemModuleFinders.java:217)
	at java.base/java.security.AccessController.doPrivileged(AccessController.java:319)

AFAICT this is coming out of UnixNativeDispatcher#readdir0 so now I'm digging into that.

dmlloyd · 2024-10-25T13:48:27Z

After a lot of printf I'm closer to a diagnosis/solution:

fdopendir is returning a valid pointer
readdir0 is using the same pointer, so nothing is being lost there
readdir is returning entries which appear to be 4-byte aligned, so I think that's valid
However the dirent's d_name field is consistently offset by 15 bytes, which is kind of weird (but not really, because char usually has one byte alignment), and that does seem to be the correct struct member offset
And, the first character of the directory name is definitely there, but at offset -1 from d_name, which is definitely screwy
So what's the common thread? A test program using fdopendir works fine. But: if I load fdopendir using dlsym, now I have the off-by-one problem. So perhaps we're somehow getting a wrong or broken fdopendir. Further testing being done.

I'm getting close!

UPDATE: I've isolated the problem to not using fdopendir64, which fixes the issue. So I think a little research is needed around whether we should be detecting 64 versions on MacOS x64.

UPDATE to the UPDATE: I've been stuck on what turns out to be a known problem with xcode 16: https://bugs.openjdk.org/browse/JDK-8340341 - hopefully I've got that solved now and we'll see what fix options we have for the actual problem.

bplb · 2024-10-25T15:40:26Z

DirectoryStream/SecureDS.java is the test for this API.

I found that after posting my question.

dmlloyd · 2024-10-25T16:07:53Z

OK the new version doesn't use dlsym on Mac OS for openat, fstatat, and fdopendir, all of which have different CPU-dependent names. This is the simplest change I could think of that solves the problem.

dmlloyd · 2024-10-25T16:44:08Z

There is no noreg-* label on the issue. Is this change covered by an existing test?

DirectoryStream/SecureDS.java is the test for this API. Right now it only runs on Linux (it used to run on Solaris too). If support is extended to macOS then we'll need to change this test to use @requires and drop the existing check in the test main.

I think the existing test will work still, because I don't see anything that restricts the OS, and the instanceof test should return true now on macos, right?

dmlloyd · 2024-10-25T16:49:39Z

There is no noreg-* label on the issue. Is this change covered by an existing test?

DirectoryStream/SecureDS.java is the test for this API. Right now it only runs on Linux (it used to run on Solaris too). If support is extended to macOS then we'll need to change this test to use @requires and drop the existing check in the test main.

I think the existing test will work still, because I don't see anything that restricts the OS, and the instanceof test should return true now on macos, right?

Never mind, I see: it will pass if everything is working, but it will also pass on macos if it's not working. So, I'll go ahead and make that change.

bplb · 2024-10-25T16:52:07Z

I think you will need to add the issue ID to the @bug tag of the test.

… logic

bplb · 2024-10-25T19:41:14Z

The build now succeeds and the test passes on macOS-x64. Testing of other platforms is in progress.

bplb · 2024-10-25T23:17:55Z

Build and test succeeded on Linux-{aarch64,x64}, macOS-{aarch64,x64}, and Windows-x64.

bplb · 2024-10-25T23:26:39Z

/reviewers 2 reviewer

openjdk · 2024-10-25T23:26:59Z

@bplb
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 2 Reviewers).

bplb

Approved. Please do not integrate until @AlanBateman has also approved, thanks.

dmlloyd · 2024-10-28T13:03:46Z

Looking at related bug history, https://bugs.openjdk.org/browse/JDK-8214078 was considered a Bug not an Enhancement. What is the distinction? Is it due to being supported on other architectures on the same OS versus being unsupported by OS?

Another question I had was: does this being an Enhancement affect backportability?

AlanBateman

I think this looks okay.

It changes the Linux port from using futimesat to futimes. This should be okay as futimes has been available for a long time.

Change the macOS to not use dlsym is okay too, it had to use dlsym in the original port.

The update to the test looks okay.

AlanBateman · 2024-10-28T16:43:21Z

Looking at related bug history, https://bugs.openjdk.org/browse/JDK-8214078 was considered a Bug not an Enhancement. What is the distinction? Is it due to being supported on other architectures on the same OS versus being unsupported by OS?

Another question I had was: does this being an Enhancement affect backportability?

SecureDirectoryStream is optional and this JBS issue is add support on macOS. So I think Enhancement is the right issue type. I don't think we can classify as a bug, it's just that this feature couldn't be implemented on macOS when the original port was done in 7u4.

I'm not involved in update releases.

dmlloyd · 2024-10-28T16:55:53Z

/integrate

openjdk · 2024-10-28T16:57:01Z

@dmlloyd
Your change (at version 6167680) is now ready to be sponsored by a Committer.

bplb · 2024-10-28T16:58:16Z

/sponsor

openjdk · 2024-10-28T16:58:48Z

Going to push as commit 9f6d5b4.
Since your change was applied there have been 32 commits pushed to the master branch:

1341b81: 8341666: FileInputStream doesn't support readAllBytes() or readNBytes(int) on pseudo devices
52382e2: 8338021: Support new unsigned and saturating vector operators in VectorAPI
e659d9d: 8342975: C2: Micro-optimize PhaseIdealLoop::Dominators()
9f6211b: 8341371: CDS cannot load archived heap objects with -XX:+UseSerialGC -XX:-UseCompressedOops
120a935: 8342561: Metaspace for generated reflection classes is no longer needed
d5fb6b4: 8339939: [JVMCI] Don't compress abstract and interface Klasses
a5ad974: 8343056: C2: Micro-optimize Node lists grow
ec06187: 8034066: Incorrect alignment in the "Code" section for "-c -XDdetails" options
eb3669a: 8340796: Use a consistent order when loading cxq and EntryList
0e3fc93: 8342083: Make a few fields in FileSystemPreferences final
... and 22 more: https://git.openjdk.org/jdk/compare/d1540e2a49c7a41eb771fc9896c367187d070dec...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-10-28T16:58:59Z

@bplb @dmlloyd Pushed as commit 9f6d5b4.

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

Simpler version that does not break arm32, hopefully

Loading
Loading status checks…

8bb31fd

openjdk bot added the rfr label Oct 24, 2024

openjdk bot added the nio label Oct 24, 2024

Use 64-bit variation when 64-bit inodes are detected on macos x86

Loading
Loading status checks…

4572704

dmlloyd added 2 commits October 25, 2024 11:53

Update jtreg SecureDS test to use @requires instead of instanceof…

Loading
Loading status checks…

cff1994

… logic

Add bug ID to test

Loading
Loading status checks…

6167680

dmlloyd changed the title ~~8343020: SecureDirectoryStream not supported on MacOS~~ 8343020: (fs) Add support for SecureDirectoryStream on macOS Oct 25, 2024

bplb approved these changes Oct 25, 2024

View reviewed changes

AlanBateman approved these changes Oct 28, 2024

View reviewed changes

openjdk bot added the ready label Oct 28, 2024

openjdk bot added the sponsor label Oct 28, 2024

openjdk bot added the integrated label Oct 28, 2024

openjdk bot closed this Oct 28, 2024

openjdk bot removed ready rfr sponsor labels Oct 28, 2024

dmlloyd deleted the futimesat branch October 28, 2024 16:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8343020: (fs) Add support for SecureDirectoryStream on macOS #21696

8343020: (fs) Add support for SecureDirectoryStream on macOS #21696

dmlloyd commented Oct 24, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Oct 24, 2024

openjdk bot commented Oct 24, 2024 •

edited

Loading

openjdk bot commented Oct 24, 2024

mlbridge bot commented Oct 24, 2024 •

edited

Loading

bplb commented Oct 24, 2024

bplb commented Oct 25, 2024

AlanBateman commented Oct 25, 2024

AlanBateman commented Oct 25, 2024 •

edited

Loading

AlanBateman commented Oct 25, 2024

dmlloyd commented Oct 25, 2024

dmlloyd commented Oct 25, 2024

dmlloyd commented Oct 25, 2024

dmlloyd commented Oct 25, 2024 •

edited

Loading

bplb commented Oct 25, 2024

dmlloyd commented Oct 25, 2024

dmlloyd commented Oct 25, 2024

dmlloyd commented Oct 25, 2024

bplb commented Oct 25, 2024

bplb commented Oct 25, 2024 •

edited

Loading

bplb commented Oct 25, 2024

bplb commented Oct 25, 2024

openjdk bot commented Oct 25, 2024

bplb left a comment

dmlloyd commented Oct 28, 2024

AlanBateman left a comment

AlanBateman commented Oct 28, 2024

dmlloyd commented Oct 28, 2024

openjdk bot commented Oct 28, 2024

bplb commented Oct 28, 2024

openjdk bot commented Oct 28, 2024

openjdk bot commented Oct 28, 2024

8343020: (fs) Add support for SecureDirectoryStream on macOS #21696

8343020: (fs) Add support for SecureDirectoryStream on macOS #21696

Conversation

dmlloyd commented Oct 24, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Oct 24, 2024

openjdk bot commented Oct 24, 2024 • edited Loading

openjdk bot commented Oct 24, 2024

mlbridge bot commented Oct 24, 2024 • edited Loading

Webrevs

bplb commented Oct 24, 2024

bplb commented Oct 25, 2024

AlanBateman commented Oct 25, 2024

AlanBateman commented Oct 25, 2024 • edited Loading

AlanBateman commented Oct 25, 2024

dmlloyd commented Oct 25, 2024

dmlloyd commented Oct 25, 2024

dmlloyd commented Oct 25, 2024

dmlloyd commented Oct 25, 2024 • edited Loading

bplb commented Oct 25, 2024

dmlloyd commented Oct 25, 2024

dmlloyd commented Oct 25, 2024

dmlloyd commented Oct 25, 2024

bplb commented Oct 25, 2024

bplb commented Oct 25, 2024 • edited Loading

bplb commented Oct 25, 2024

bplb commented Oct 25, 2024

openjdk bot commented Oct 25, 2024

bplb left a comment

Choose a reason for hiding this comment

dmlloyd commented Oct 28, 2024

AlanBateman left a comment

Choose a reason for hiding this comment

AlanBateman commented Oct 28, 2024

dmlloyd commented Oct 28, 2024

openjdk bot commented Oct 28, 2024

bplb commented Oct 28, 2024

openjdk bot commented Oct 28, 2024

openjdk bot commented Oct 28, 2024

dmlloyd commented Oct 24, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Oct 24, 2024 •

edited

Loading

mlbridge bot commented Oct 24, 2024 •

edited

Loading

AlanBateman commented Oct 25, 2024 •

edited

Loading

dmlloyd commented Oct 25, 2024 •

edited

Loading

bplb commented Oct 25, 2024 •

edited

Loading