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: [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]
