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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
