You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "holdenk (via GitHub)" <gi...@apache.org> on 2026/09/26 00:23:46 UTC
[PR] [SPARK-59756][CORE] Make Utils.chmod700 atomic on POSIX FS [spark]
holdenk opened a new pull request, #59061:
URL: https://github.com/apache/spark/pull/59061
### What changes were proposed in this pull request?
`Utils.chmod700` now makes a single `Files.setPosixFilePermissions(path, "rwx------")` call on POSIX filesystems, instead of six `File.setReadable/setWritable/setExecutable` calls.
On non-POSIX filesystems (e.g. Windows) the `File.set*` sequence is kept as a best-effort, non-atomic fallback, but reordered: all permissions are stripped first and owner permissions granted after, so the file is locked down as early as possible and no intermediate state grants group/other access.
Review notes:
- `chmod700` guards the empty path explicitly: `new File("").toPath` resolves to the process working directory in NIO, and the old `File.set*` sequence returned `false` for it; the guard preserves that behavior instead of chmod-ing the cwd.
- A `SecurityException` from the POSIX attempt now becomes `false` (matching the documented `@return`) instead of propagating; only reachable under a `SecurityManager` that denies file write.
### Why are the changes needed?
Each `File.set*` call is a non-atomic stat-then-chmod, so concurrent `chmod700` calls on the same path (e.g. Workers sharing a local root dir in local-cluster mode) can interleave into a broken permission set. On POSIX filesystems a single `chmod` is atomic.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Added tests to `UtilsSuite`: concurrent `chmod700` calls on the same path always leave `rwx------` (POSIX-only), a nonexistent path returns `false`, and an empty path returns `false`. Ran the full `core` module build and test suite locally (GREEN).
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Cursor
Made with [Cursor](https://cursor.com)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59756][CORE] Make Utils.chmod700 atomic on POSIX FS [spark]
Posted by "sarutak (via GitHub)" <gi...@apache.org>.
sarutak commented on code in PR #59061:
URL: https://github.com/apache/spark/pull/59061#discussion_r4281977394
##########
core/src/main/scala/org/apache/spark/util/Utils.scala:
##########
@@ -223,19 +224,52 @@ private[spark] object Utils
writeByteBufferImpl(bb, out.write)
}
+ // fromString returns a mutable HashSet; wrap it so no future caller can mutate the shared set.
+ private val chmod700Perms =
+ Collections.unmodifiableSet(PosixFilePermissions.fromString("rwx------"))
+
/**
* JDK equivalent of `chmod 700 file`.
*
+ * On POSIX filesystems this is a single `chmod` call, which is atomic: concurrent calls on
+ * the same path (e.g. Workers sharing a local root dir in local-cluster mode) cannot
+ * interleave into a broken permission set, unlike the `File.set*` fallback below, each call
+ * of which is a non-atomic stat-then-chmod.
+ *
* @param file the file whose permissions will be modified
* @return true if the permissions were successfully changed, false otherwise.
*/
def chmod700(file: File): Boolean = {
- file.setReadable(false, false) &&
- file.setReadable(true, true) &&
- file.setWritable(false, false) &&
- file.setWritable(true, true) &&
- file.setExecutable(false, false) &&
- file.setExecutable(true, true)
+ if (file.getPath.isEmpty) {
+ // The NIO empty path resolves to the process working directory; the File.set* sequence
+ // returned false for it, so keep that behavior rather than chmod-ing the cwd.
+ return false
+ }
+ try {
+ Files.setPosixFilePermissions(file.toPath, chmod700Perms)
Review Comment:
`setPosixFilePermissions` writes exactly the 9 rwx bits, so this now clears setgid/setuid/sticky. In JDK 17, [UnixFileAttributeViews.java L296-L299](https://github.com/openjdk/jdk17u/blob/0b592b7f04aae6cec666345be37c1456845e6e0d/src/java.base/unix/classes/sun/nio/fs/UnixFileAttributeViews.java#L296-L299) passes [UnixFileModeAttribute.toUnixMode](https://github.com/openjdk/jdk17u/blob/0b592b7f04aae6cec666345be37c1456845e6e0d/src/java.base/unix/classes/sun/nio/fs/UnixFileModeAttribute.java#L48-L65) (built from the 9 permission bits only) straight to `chmod` in [setMode](https://github.com/openjdk/jdk17u/blob/0b592b7f04aae6cec666345be37c1456845e6e0d/src/java.base/unix/classes/sun/nio/fs/UnixFileAttributeViews.java#L263-L267). The old `File.set*` path kept them, because each call chmods the stat'ed mode with one bit changed (GNU `chmod 700` also keeps setgid on a directory). Checked on macOS with a `2755` directory:
1. Old sequence: `drwx--S---` (setgid kept)
2. `setPosixFilePermissions`: `drwx------` (setgid cleared)
This is reachable from `getOrCreateLocalRootDirsImpl`, for example when the `spark-*` dir is created under a setgid parent and inherits the bit. With mode 700 the group has no access anyway, so I do not expect a practical impact. But it makes the Scaladoc's "JDK equivalent of `chmod 700`" and the "no user-facing change" statement slightly inaccurate. Could you mention it in the Scaladoc and the PR description? If preserving the special bits matters, `(mode & 07000) | 0700` through `Files.setAttribute(path, "unix:mode", ...)` is still a single chmod, but documenting it seems sufficient to me.
##########
core/src/main/scala/org/apache/spark/util/Utils.scala:
##########
@@ -223,19 +224,52 @@ private[spark] object Utils
writeByteBufferImpl(bb, out.write)
}
+ // fromString returns a mutable HashSet; wrap it so no future caller can mutate the shared set.
+ private val chmod700Perms =
+ Collections.unmodifiableSet(PosixFilePermissions.fromString("rwx------"))
+
/**
* JDK equivalent of `chmod 700 file`.
*
+ * On POSIX filesystems this is a single `chmod` call, which is atomic: concurrent calls on
+ * the same path (e.g. Workers sharing a local root dir in local-cluster mode) cannot
+ * interleave into a broken permission set, unlike the `File.set*` fallback below, each call
+ * of which is a non-atomic stat-then-chmod.
+ *
* @param file the file whose permissions will be modified
* @return true if the permissions were successfully changed, false otherwise.
*/
def chmod700(file: File): Boolean = {
- file.setReadable(false, false) &&
- file.setReadable(true, true) &&
- file.setWritable(false, false) &&
- file.setWritable(true, true) &&
- file.setExecutable(false, false) &&
- file.setExecutable(true, true)
+ if (file.getPath.isEmpty) {
+ // The NIO empty path resolves to the process working directory; the File.set* sequence
+ // returned false for it, so keep that behavior rather than chmod-ing the cwd.
+ return false
+ }
+ try {
+ Files.setPosixFilePermissions(file.toPath, chmod700Perms)
+ true
+ } catch {
+ case _: UnsupportedOperationException =>
+ // Non-POSIX filesystem (e.g. Windows): best-effort, non-atomic fallback. Remove all
+ // permissions first so the file is locked down as early as possible, then grant the
+ // owner permissions; no intermediate state grants group/other access.
+ val succeeded =
+ file.setReadable(false, false) &&
+ file.setWritable(false, false) &&
+ file.setExecutable(false, false) &&
+ file.setReadable(true, true) &&
+ file.setWritable(true, true) &&
+ file.setExecutable(true, true)
+ if (!succeeded) {
+ // Structural on Windows: File.setReadable(false, false) always fails there (the
+ // filesystem has no read permission to revoke), so this fires on every call.
+ logWarning(log"Failed to set permissions to 700 on ${MDC(PATH, file.getAbsolutePath)}")
Review Comment:
As the comment says, this fires on every call on Windows (the native `WinNTFileSystem.setPermission` returns `enable` for READ/EXECUTE, see [WinNTFileSystem_md.c L473-L476](https://github.com/openjdk/jdk17u/blob/0b592b7f04aae6cec666345be37c1456845e6e0d/src/java.base/windows/native/libjava/WinNTFileSystem_md.c#L473-L476) in JDK 17, so `setReadable(false, false)` is always `false`). It is hit once per local root dir, per executor app dir in `Worker`, per app store in `HistoryServerDiskManager`, and in `KVUtils`, and there is nothing the user can do about it. The old code was silent here. Would it make sense to log this structural case only once per JVM (or at DEBUG), and keep the WARN with the throwable for the `NonFatal` branch, which is the case my earlier comment was about?
##########
core/src/test/scala/org/apache/spark/util/UtilsSuite.scala:
##########
@@ -572,6 +573,73 @@ class UtilsSuite extends SparkFunSuite with ResetSystemProperties {
assert(!Utils.createDirectory(scenario8))
}
+ test("chmod700 leaves rwx------ under concurrent calls on the same path") {
+ // Workers in local-cluster mode share local root dirs and can chmod the same
+ // per-application directory concurrently; the stat-then-chmod File.set* sequence
+ // interleaves into broken permission sets, so chmod700 must be atomic. A post-join
Review Comment:
I need to correct [my earlier "Agreed"](https://github.com/apache/spark/pull/59061#discussion_r4172425733) on the post-join discussion: the premise that the final mode is `rwx------` either way does not hold, so this comment is inaccurate.
Each `File.set*` call is a read-modify-write: the native `UnixFileSystem.setPermission` stats the mode, then calls `chmod(path, mode | amode)` or `chmod(path, mode & ~amode)` ([UnixFileSystem_md.c L211-L216](https://github.com/openjdk/jdk17u/blob/0b592b7f04aae6cec666345be37c1456845e6e0d/src/java.base/unix/native/libjava/UnixFileSystem_md.c#L211-L216) in JDK 17). A grant computed from a stale stat can be the last write and bring back an earlier revoke. Example, starting at 700:
1. Thread B runs `setWritable(false, false)`: 700 to 500.
2. Thread A, in its final `setExecutable(true, true)`, stats 500 and is preempted before its chmod.
3. Thread B finishes its remaining calls: the mode is back at 700.
4. Thread A writes 500 | u+x = 500. The final mode is `r-x------`.
I checked this with a standalone harness (the old six call sequence vs `setPosixFilePermissions`):
- The PR's test shape on the old code failed the post-join assert at line 620 in 4 of 5 runs, and the observer flagged intermediate states in 5 of 5 runs.
- Post-join only, 2000 rounds of 8 threads x 50 calls on the old code: 170 rounds ended in `-wx------`, `r-x------` or `--x------`.
- The new code: 0 failures in all of the above.
So the post-join assert is a weak but real guard, and the broken end state is the bug the description's "Why" section describes. The observer is still the right addition, since it catches the transient states far more reliably.
##########
core/src/main/scala/org/apache/spark/util/Utils.scala:
##########
@@ -223,19 +224,52 @@ private[spark] object Utils
writeByteBufferImpl(bb, out.write)
}
+ // fromString returns a mutable HashSet; wrap it so no future caller can mutate the shared set.
+ private val chmod700Perms =
+ Collections.unmodifiableSet(PosixFilePermissions.fromString("rwx------"))
+
/**
* JDK equivalent of `chmod 700 file`.
*
+ * On POSIX filesystems this is a single `chmod` call, which is atomic: concurrent calls on
+ * the same path (e.g. Workers sharing a local root dir in local-cluster mode) cannot
+ * interleave into a broken permission set, unlike the `File.set*` fallback below, each call
+ * of which is a non-atomic stat-then-chmod.
+ *
* @param file the file whose permissions will be modified
* @return true if the permissions were successfully changed, false otherwise.
*/
def chmod700(file: File): Boolean = {
- file.setReadable(false, false) &&
- file.setReadable(true, true) &&
- file.setWritable(false, false) &&
- file.setWritable(true, true) &&
- file.setExecutable(false, false) &&
- file.setExecutable(true, true)
+ if (file.getPath.isEmpty) {
+ // The NIO empty path resolves to the process working directory; the File.set* sequence
+ // returned false for it, so keep that behavior rather than chmod-ing the cwd.
+ return false
+ }
+ try {
+ Files.setPosixFilePermissions(file.toPath, chmod700Perms)
+ true
+ } catch {
+ case _: UnsupportedOperationException =>
+ // Non-POSIX filesystem (e.g. Windows): best-effort, non-atomic fallback. Remove all
Review Comment:
On Windows, the first step `setReadable(false, false)` always fails and `&&` short-circuits, so nothing is stripped and none of the later steps run. That is what the comment at line 264 says, so these two comments contradict each other. Also, on a filesystem where the sequence did run, the intermediate states would still keep the existing group/other bits until each one is cleared; the accurate property is "never adds group/other bits", which the old order already had. Could you reword this comment and the matching sentence in the PR description, or keep the original order to make the diff smaller?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59756][CORE] Make Utils.chmod700 atomic on POSIX FS [spark]
Posted by "sarutak (via GitHub) via reviews" <re...@spark.apache.org>.
sarutak commented on code in PR #59061:
URL: https://github.com/apache/spark/pull/59061#discussion_r4439038787
##########
core/src/main/scala/org/apache/spark/util/Utils.scala:
##########
@@ -223,19 +224,52 @@ private[spark] object Utils
writeByteBufferImpl(bb, out.write)
}
+ // fromString returns a mutable HashSet; wrap it so no future caller can mutate the shared set.
+ private val chmod700Perms =
+ Collections.unmodifiableSet(PosixFilePermissions.fromString("rwx------"))
+
/**
* JDK equivalent of `chmod 700 file`.
*
+ * On POSIX filesystems this is a single `chmod` call, which is atomic: concurrent calls on
+ * the same path (e.g. Workers sharing a local root dir in local-cluster mode) cannot
+ * interleave into a broken permission set, unlike the `File.set*` fallback below, each call
+ * of which is a non-atomic stat-then-chmod.
+ *
* @param file the file whose permissions will be modified
* @return true if the permissions were successfully changed, false otherwise.
*/
def chmod700(file: File): Boolean = {
- file.setReadable(false, false) &&
- file.setReadable(true, true) &&
- file.setWritable(false, false) &&
- file.setWritable(true, true) &&
- file.setExecutable(false, false) &&
- file.setExecutable(true, true)
+ if (file.getPath.isEmpty) {
+ // The NIO empty path resolves to the process working directory; the File.set* sequence
+ // returned false for it, so keep that behavior rather than chmod-ing the cwd.
+ return false
+ }
+ try {
+ Files.setPosixFilePermissions(file.toPath, chmod700Perms)
+ true
+ } catch {
+ case _: UnsupportedOperationException =>
+ // Non-POSIX filesystem (e.g. Windows): best-effort, non-atomic fallback. Remove all
Review Comment:
I think the non-short-circuit `&` you pushed is the right choice, and `||` would not work here. `||` stops at the first call that returns true. On Windows, the native `WinNTFileSystem.setPermission` returns `enable` as is for READ and EXECUTE ([WinNTFileSystem_md.c L473-L476](https://github.com/openjdk/jdk17u/blob/0b592b7f04aae6cec666345be37c1456845e6e0d/src/java.base/windows/native/libjava/WinNTFileSystem_md.c#L473-L476) in JDK 17), so `setReadable(true, true)` always returns true. For WRITE it skips directories and returns false ([L495-L496](https://github.com/openjdk/jdk17u/blob/0b592b7f04aae6cec666345be37c1456845e6e0d/src/java.base/windows/native/libjava/WinNTFileSystem_md.c#L495-L496)), and on a regular file it sets or clears `FILE_ATTRIBUTE_READONLY` ([L498-L503](https://github.com/openjdk/jdk17u/blob/0b592b7f04aae6cec666345be37c1456845e6e0d/src/java.base/windows/native/libjava/WinNTFileSystem_md.c#L498-L503)). So for a directory `||` would return true without changing anyth
ing, and for a regular file it would stop right after `setWritable(false, false)` and leave the file read-only. With `&`, every step runs and the result is true only if all of them succeed, which matches the `@return`. For directories, which all current callers pass, `&` behaves exactly like the old `&&`: no writes, and the result is false.
##########
core/src/main/scala/org/apache/spark/util/Utils.scala:
##########
@@ -223,19 +224,53 @@ private[spark] object Utils
writeByteBufferImpl(bb, out.write)
}
+ // fromString returns a mutable HashSet; wrap it so no future caller can mutate the shared set.
+ private val chmod700Perms =
+ Collections.unmodifiableSet(PosixFilePermissions.fromString("rwx------"))
+
/**
* JDK equivalent of `chmod 700 file`.
*
+ * On POSIX filesystems this is a single `chmod` call, which is atomic: concurrent calls on
+ * the same path (e.g. Workers sharing a local root dir in local-cluster mode) cannot
Review Comment:
Is there a path that actually runs `chmod700` on the same path concurrently? As far as I can tell, every caller chmods a directory it has just created under a unique name: `executor-<UUID>` in `Worker`, `spark-<UUID>` in `getOrCreateLocalRootDirsImpl` (and `getOrCreateLocalRootDirs` caches the result under `this.synchronized`, so Workers in the same JVM share one root but chmod it only once), `appstore-<UUID>` in `HistoryServerDiskManager`, and in `KVUtils` either a `listing.*db` under a fresh `spark-ui-<UUID>` (live UI) or under the History Server's own store path (one per History Server). If that is right, the "e.g." here, the test comment at UtilsSuite.scala line 578, and the "Why" section describe a race that current callers cannot hit. The change is still worth it as described in the JIRA (one atomic call instead of six), so maybe phrase it as "concurrent calls on the same path cannot interleave" without the local-cluster example, unless there is a concrete case I am missing.
##########
core/src/main/scala/org/apache/spark/util/Utils.scala:
##########
@@ -223,19 +224,53 @@ private[spark] object Utils
writeByteBufferImpl(bb, out.write)
}
+ // fromString returns a mutable HashSet; wrap it so no future caller can mutate the shared set.
+ private val chmod700Perms =
+ Collections.unmodifiableSet(PosixFilePermissions.fromString("rwx------"))
+
/**
* JDK equivalent of `chmod 700 file`.
*
+ * On POSIX filesystems this is a single `chmod` call, which is atomic: concurrent calls on
+ * the same path (e.g. Workers sharing a local root dir in local-cluster mode) cannot
+ * interleave into a broken permission set, unlike the `File.set*` fallback below, each call
+ * of which is a non-atomic stat-then-chmod. The POSIX call writes exactly the nine rwx bits,
+ * so it also clears setuid, setgid, and sticky bits; the fallback preserves them.
Review Comment:
Thanks for adding this. Nit: the fallback only runs where there is no POSIX view (in practice Windows), so there are no such bits there to preserve. What the reader needs is the contrast with the previous implementation, e.g. "..., which the previous `File.set*` implementation kept." Similarly for the fallback comment at line 254: on Windows, `setReadable`/`setExecutable` do nothing ([WinNTFileSystem_md.c L473-L476](https://github.com/openjdk/jdk17u/blob/0b592b7f04aae6cec666345be37c1456845e6e0d/src/java.base/windows/native/libjava/WinNTFileSystem_md.c#L473-L476) in JDK 17) and `setWritable` ignores directories ([L495-L496](https://github.com/openjdk/jdk17u/blob/0b592b7f04aae6cec666345be37c1456845e6e0d/src/java.base/windows/native/libjava/WinNTFileSystem_md.c#L495-L496)), so for a directory (every current caller) the fallback changes nothing and returns false. Saying that directly would be clearer than "revoke ... where supported".
##########
core/src/test/scala/org/apache/spark/util/UtilsSuite.scala:
##########
@@ -573,6 +574,71 @@ class UtilsSuite extends SparkFunSuite with ResetSystemProperties {
assert(!Utils.createDirectory(scenario8))
}
+ test("chmod700 leaves rwx------ under concurrent calls on the same path") {
+ // Workers in local-cluster mode share local root dirs and can chmod the same
+ // per-application directory concurrently; the stat-then-chmod File.set* sequence
+ // interleaves into broken permission sets, so chmod700 must be atomic. Check both
+ // the final mode and samples taken during the race: with an atomic chmod every sample
+ // is the initial or the final mode, and anything else is a non-atomic interleaving.
+ assume(FileSystems.getDefault.supportedFileAttributeViews().contains("posix"))
+ withTempDir { dir =>
+ val initialPerms = PosixFilePermissions.fromString("rwxr-xr-x")
+ val expectedPerms = PosixFilePermissions.fromString("rwx------")
+ // Start from a permissive mode so the assertions cannot pass vacuously under umask 077.
+ Files.setPosixFilePermissions(dir.toPath, initialPerms)
+ @volatile var observing = true
+ // Written only by the observer and read after observer.join(), which synchronizes.
+ var brokenStates = Set.empty[java.util.Set[PosixFilePermission]]
+ var observerError: Throwable = null
+ val observer = new Thread(() => {
+ try {
+ while (observing) {
+ val perms = Files.getPosixFilePermissions(dir.toPath)
+ if (perms != initialPerms && perms != expectedPerms) {
+ brokenStates += perms
+ }
+ }
+ } catch {
+ case t: Throwable => observerError = t
+ }
+ })
+ observer.start()
+ try {
+ val numThreads = 4
+ (1 to 10).foreach { _ =>
+ // Align the thread starts to widen the race window.
+ val barrier = new CyclicBarrier(numThreads)
+ val threads = (1 to numThreads).map { _ =>
+ new Thread(() => {
+ barrier.await()
+ (1 to 20).foreach(_ => Utils.chmod700(dir))
Review Comment:
Nit: no test checks that `chmod700` returns `true` on POSIX; the other two tests only check `false`. A single `assert(Utils.chmod700(dir))` on the main thread (e.g. before the race) would cover it. An assert inside these worker threads would not fail the test.
##########
core/src/main/scala/org/apache/spark/util/Utils.scala:
##########
@@ -223,19 +224,53 @@ private[spark] object Utils
writeByteBufferImpl(bb, out.write)
}
+ // fromString returns a mutable HashSet; wrap it so no future caller can mutate the shared set.
Review Comment:
Nit: `fromString` actually builds an `EnumSet` ([PosixFilePermissions.java L129](https://github.com/openjdk/jdk17u/blob/0b592b7f04aae6cec666345be37c1456845e6e0d/src/java.base/share/classes/java/nio/file/attribute/PosixFilePermissions.java#L129) in JDK 17). My earlier comment said `HashSet`, sorry about that. "returns a mutable set" would be accurate.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59756][CORE] Make Utils.chmod700 atomic on POSIX FS [spark]
Posted by "sarutak (via GitHub) via reviews" <re...@spark.apache.org>.
sarutak closed pull request #59061: [SPARK-59756][CORE] Make Utils.chmod700 atomic on POSIX FS
URL: https://github.com/apache/spark/pull/59061
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59756][CORE] Make Utils.chmod700 atomic on POSIX FS [spark]
Posted by "sarutak (via GitHub)" <gi...@apache.org>.
sarutak commented on code in PR #59061:
URL: https://github.com/apache/spark/pull/59061#discussion_r4130813220
##########
core/src/main/scala/org/apache/spark/util/Utils.scala:
##########
@@ -223,19 +224,41 @@ private[spark] object Utils
writeByteBufferImpl(bb, out.write)
}
+ private val chmod700Perms = PosixFilePermissions.fromString("rwx------")
Review Comment:
`PosixFilePermissions.fromString(...)` returns a mutable `HashSet`. It is safe as used here (`private val`, no mutation, and `Files.setPosixFilePermissions` copies defensively), but wrapping it in an immutable set would make the intent explicit and guard against a future accidental mutation.
```scala
private val chmod700Perms =
java.util.Collections.unmodifiableSet(PosixFilePermissions.fromString("rwx------"))
```
##########
core/src/test/scala/org/apache/spark/util/UtilsSuite.scala:
##########
@@ -572,6 +573,37 @@ class UtilsSuite extends SparkFunSuite with ResetSystemProperties {
assert(!Utils.createDirectory(scenario8))
}
+ test("chmod700 leaves rwx------ under concurrent calls on the same path") {
+ // Workers in local-cluster mode share local root dirs and can chmod the same
+ // per-application directory concurrently; the stat-then-chmod File.set* sequence
+ // interleaves into broken permission sets, so chmod700 must be atomic.
+ assume(FileSystems.getDefault.supportedFileAttributeViews().contains("posix"))
+ withTempDir { dir =>
+ // Start from a permissive mode so the assertion cannot pass vacuously under umask 077.
+ Files.setPosixFilePermissions(dir.toPath, PosixFilePermissions.fromString("rwxr-xr-x"))
+ (1 to 10).foreach { _ =>
+ val threads = (1 to 4).map { _ =>
Review Comment:
Detecting the race is inherently non-deterministic. Even the non-atomic old implementation can converge to `rwx------` depending on timing, so this test would not reliably fail against it (the repetition count does raise the detection probability, so it still has regression value). Aligning thread start with a `CyclicBarrier` would widen the race window and improve both detection power and flakiness resistance.
##########
core/src/main/scala/org/apache/spark/util/Utils.scala:
##########
@@ -223,19 +224,41 @@ private[spark] object Utils
writeByteBufferImpl(bb, out.write)
}
+ private val chmod700Perms = PosixFilePermissions.fromString("rwx------")
+
/**
* JDK equivalent of `chmod 700 file`.
*
+ * On POSIX filesystems this is a single `chmod` call, which is atomic: concurrent calls on
+ * the same path (e.g. Workers sharing a local root dir in local-cluster mode) cannot
+ * interleave into a broken permission set, unlike the `File.set*` fallback below, each call
+ * of which is a non-atomic stat-then-chmod.
+ *
* @param file the file whose permissions will be modified
* @return true if the permissions were successfully changed, false otherwise.
*/
def chmod700(file: File): Boolean = {
- file.setReadable(false, false) &&
- file.setReadable(true, true) &&
- file.setWritable(false, false) &&
- file.setWritable(true, true) &&
- file.setExecutable(false, false) &&
- file.setExecutable(true, true)
+ if (file.getPath.isEmpty) {
+ // The NIO empty path resolves to the process working directory; the File.set* sequence
+ // returned false for it, so keep that behavior rather than chmod-ing the cwd.
+ return false
+ }
+ try {
+ Files.setPosixFilePermissions(file.toPath, chmod700Perms)
+ true
+ } catch {
+ case _: UnsupportedOperationException =>
+ // Non-POSIX filesystem (e.g. Windows): best-effort, non-atomic fallback. Remove all
+ // permissions first so the file is locked down as early as possible, then grant the
+ // owner permissions; no intermediate state grants group/other access.
+ file.setReadable(false, false) &&
+ file.setWritable(false, false) &&
+ file.setExecutable(false, false) &&
+ file.setReadable(true, true) &&
+ file.setWritable(true, true) &&
+ file.setExecutable(true, true)
+ case NonFatal(_) => false
Review Comment:
`chmod700` emits no log on any path that returns `false` (the `NonFatal` catch and a partial failure of the non-POSIX fallback). The old implementation was also silent and all callers (`getOrCreateLocalRootDirs`, `Worker`, `HistoryServerDiskManager`, `KVUtils`) ignore the return value, so this is not a new regression — but since granting `700` is a security-boundary operation, a `logWarning` on the `NonFatal` path (which swallows e.g. `NoSuchFileException` and `SecurityException`) and on fallback failure would keep a permission-lockdown failure from going silent in production.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59756][CORE] Make Utils.chmod700 atomic on POSIX FS [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #59061:
URL: https://github.com/apache/spark/pull/59061#discussion_r4172216633
##########
core/src/test/scala/org/apache/spark/util/UtilsSuite.scala:
##########
@@ -572,6 +573,37 @@ class UtilsSuite extends SparkFunSuite with ResetSystemProperties {
assert(!Utils.createDirectory(scenario8))
}
+ test("chmod700 leaves rwx------ under concurrent calls on the same path") {
+ // Workers in local-cluster mode share local root dirs and can chmod the same
+ // per-application directory concurrently; the stat-then-chmod File.set* sequence
+ // interleaves into broken permission sets, so chmod700 must be atomic.
+ assume(FileSystems.getDefault.supportedFileAttributeViews().contains("posix"))
+ withTempDir { dir =>
+ // Start from a permissive mode so the assertion cannot pass vacuously under umask 077.
+ Files.setPosixFilePermissions(dir.toPath, PosixFilePermissions.fromString("rwxr-xr-x"))
+ (1 to 10).foreach { _ =>
+ val threads = (1 to 4).map { _ =>
+ new Thread(() => (1 to 20).foreach(_ => Utils.chmod700(dir)))
+ }
+ threads.foreach(_.start())
+ threads.foreach(_.join())
+ assert(
Review Comment:
**Nit (P3):** This assertion runs only after every thread has joined, and the post-join mode is deterministically `rwx------` whether or not chmod700 is atomic: each thread completes all its calls, and each call ends on owner grants, so for every permission bit the globally last operation is a grant. A revert to the old non-atomic `File.set*` sequence would still pass this test, so it does not actually guard the atomic-vs-non-atomic property it is named for (it does still guard that chmod700 lands at `rwx------`). The `CyclicBarrier` suggested in the other thread would not change this either, since it only aligns thread starts while the assertion still samples the quiescent post-join state. To catch the transient broken state you would need to sample permissions from an observer thread during the race; otherwise consider reframing this as an end-state check and renaming it so the intent matches what it verifies.
**Verification:**
- **Regression:** A concurrent chmod700 scenario whose assertion fails whenever the final directory mode is anything other than rwx------.
- **Inspection:** The test name and comment no longer claim to detect the non-atomic mid-race interleaving that a post-join assertion cannot observe.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59756][CORE] Make Utils.chmod700 atomic on POSIX FS [spark]
Posted by "sarutak (via GitHub) via reviews" <re...@spark.apache.org>.
sarutak commented on PR #59061:
URL: https://github.com/apache/spark/pull/59061#issuecomment-6725307635
**Merge Summary:**
- merged into master https://github.com/apache/spark/commit/76ddf554d0ea791a7a15ea203af94dc7d3dd650c
- merged into branch-4.x https://github.com/apache/spark/commit/a7088634f605ac0c4c8afd1a825cab929a733fcf
*Posted by `merge_spark_pr.py`*
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59756][CORE] Make Utils.chmod700 atomic on POSIX FS [spark]
Posted by "sarutak (via GitHub)" <gi...@apache.org>.
sarutak commented on code in PR #59061:
URL: https://github.com/apache/spark/pull/59061#discussion_r4172425733
##########
core/src/test/scala/org/apache/spark/util/UtilsSuite.scala:
##########
@@ -572,6 +573,37 @@ class UtilsSuite extends SparkFunSuite with ResetSystemProperties {
assert(!Utils.createDirectory(scenario8))
}
+ test("chmod700 leaves rwx------ under concurrent calls on the same path") {
+ // Workers in local-cluster mode share local root dirs and can chmod the same
+ // per-application directory concurrently; the stat-then-chmod File.set* sequence
+ // interleaves into broken permission sets, so chmod700 must be atomic.
+ assume(FileSystems.getDefault.supportedFileAttributeViews().contains("posix"))
+ withTempDir { dir =>
+ // Start from a permissive mode so the assertion cannot pass vacuously under umask 077.
+ Files.setPosixFilePermissions(dir.toPath, PosixFilePermissions.fromString("rwxr-xr-x"))
+ (1 to 10).foreach { _ =>
+ val threads = (1 to 4).map { _ =>
+ new Thread(() => (1 to 20).foreach(_ => Utils.chmod700(dir)))
+ }
+ threads.foreach(_.start())
+ threads.foreach(_.join())
+ assert(
Review Comment:
> The CyclicBarrier suggested in the other thread would not change this either, since it only aligns thread starts while the assertion still samples the quiescent post-join state.
Agreed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59756][CORE] Make Utils.chmod700 atomic on POSIX FS [spark]
Posted by "holdenk (via GitHub) via reviews" <re...@spark.apache.org>.
holdenk commented on code in PR #59061:
URL: https://github.com/apache/spark/pull/59061#discussion_r4429693676
##########
core/src/main/scala/org/apache/spark/util/Utils.scala:
##########
@@ -223,19 +224,52 @@ private[spark] object Utils
writeByteBufferImpl(bb, out.write)
}
+ // fromString returns a mutable HashSet; wrap it so no future caller can mutate the shared set.
+ private val chmod700Perms =
+ Collections.unmodifiableSet(PosixFilePermissions.fromString("rwx------"))
+
/**
* JDK equivalent of `chmod 700 file`.
*
+ * On POSIX filesystems this is a single `chmod` call, which is atomic: concurrent calls on
+ * the same path (e.g. Workers sharing a local root dir in local-cluster mode) cannot
+ * interleave into a broken permission set, unlike the `File.set*` fallback below, each call
+ * of which is a non-atomic stat-then-chmod.
+ *
* @param file the file whose permissions will be modified
* @return true if the permissions were successfully changed, false otherwise.
*/
def chmod700(file: File): Boolean = {
- file.setReadable(false, false) &&
- file.setReadable(true, true) &&
- file.setWritable(false, false) &&
- file.setWritable(true, true) &&
- file.setExecutable(false, false) &&
- file.setExecutable(true, true)
+ if (file.getPath.isEmpty) {
+ // The NIO empty path resolves to the process working directory; the File.set* sequence
+ // returned false for it, so keep that behavior rather than chmod-ing the cwd.
+ return false
+ }
+ try {
+ Files.setPosixFilePermissions(file.toPath, chmod700Perms)
+ true
+ } catch {
+ case _: UnsupportedOperationException =>
+ // Non-POSIX filesystem (e.g. Windows): best-effort, non-atomic fallback. Remove all
Review Comment:
Yeah, maybe we'll just move to `||` since we probably want to try and match-ish on windows if we can. WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org