sunchao commented on code in PR #6987: URL: https://github.com/apache/datafusion-comet/pull/6987#discussion_r4272601066
########## spark/src/main/java/org/apache/comet/NativeLibrary.java: ########## @@ -0,0 +1,138 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.comet; + +import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; +import java.nio.channels.Channels; +import java.nio.channels.SeekableByteChannel; +import java.nio.file.Files; +import java.nio.file.LinkOption; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.nio.file.StandardOpenOption; +import java.nio.file.attribute.PosixFilePermissions; +import java.nio.file.attribute.UserPrincipal; +import java.util.EnumSet; +import java.util.Map; + +/** Owns a bundled native library until it is loaded or discarded. */ +final class NativeLibrary implements AutoCloseable { + // Use a different prefix from legacy libcomet-* files: old JVMs must not clean up this directory. + private static final String DIRECTORY_PREFIX = "comet-native-"; + private final Path directory; + private final Path library; + private boolean retained; + + private NativeLibrary(Path directory, String libraryName) { + this.directory = directory; + this.library = directory.resolve(libraryName); + } + + static NativeLibrary extract(InputStream resource, String libraryName) throws IOException { + return extract(resource, libraryName, Paths.get(System.getProperty("java.io.tmpdir"))); + } + + static NativeLibrary extract(InputStream resource, String libraryName, Path tempRoot) + throws IOException { + Path root = tempRoot.toRealPath(); + checkTempRoot(root); + Path directory = + Files.createTempDirectory( + root, + DIRECTORY_PREFIX, + PosixFilePermissions.asFileAttribute(PosixFilePermissions.fromString("rwx------"))); + NativeLibrary result = new NativeLibrary(directory, libraryName); + try { + copy(resource, result.library); + return result; + } catch (IOException | RuntimeException | Error e) { + try { + result.close(); + } catch (IOException cleanupError) { + e.addSuppressed(cleanupError); + } + throw e; + } + } + + private static void checkTempRoot(Path root) throws IOException { + // A private child can still be renamed in a shared, non-sticky parent. Check every ancestor + // after resolving symlinks so another local user cannot replace any component of the load path. + // Fail closed on filesystems where these Unix protections cannot be established. + if (!root.getFileSystem().supportedFileAttributeViews().contains("unix")) { + throw new IOException("Cannot securely extract libcomet on a non-Unix filesystem"); + } + UserPrincipal owner = + root.getFileSystem() + .getUserPrincipalLookupService() + .lookupPrincipalByName(System.getProperty("user.name")); + for (Path ancestor = root; ancestor != null; ancestor = ancestor.getParent()) { + Map<String, Object> attributes = + Files.readAttributes(ancestor, "unix:uid,mode,owner", LinkOption.NOFOLLOW_LINKS); + int mode = (int) attributes.get("mode"); + boolean trustedOwner = + (int) attributes.get("uid") == 0 || owner.equals(attributes.get("owner")); Review Comment: [P2] [P2] Preserve bundled loading under secure YARN directory ownership When a YARN application runs as `alice` and the NodeManager runs as `yarn`, Spark sets `java.io.tmpdir` beneath the container working directory. Hadoop explicitly requires `yarn.nodemanager.local-dirs`, an ancestor of that directory, to be owned by the NodeManager account with mode `0755`. This predicate rejects that ancestor even when the application's own temporary directory is private. Consequently, ordinary bundled-JAR deployments that loaded successfully before this PR now fail with `Cannot unpack libcomet`, disabling Comet through the default fallback or raising an error when `spark.comet.nativeLoadRequired=true`. This affects all five supported Spark lines checked. Could the loader select a separately validated extraction root or provide a safe configurable alternative when Spark's temporary-directory ancestry is unsuitable, while retaining the private-directory and exclusive-creation protections? Evidence: Spark ExecutorRunnable sets `-Djava.io.tmpdir` to `${PWD}/tmp` in tags v3.4.3, v3.5.9, v4.0.4, v4.1.3, and v4.2.0. Example: https://github.com/apache/spark/blob/v4.1.3/resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/ExecutorRunnable.scala#L174 . Hadoop's secure-container documentation requires NodeManager ownership and 0755 permissions: https://github.com/apache/hadoop/blob/rel/release-3.4.2/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-site/src/site/markdown/SecureContainer.md#L37 . `check_nm_local_dir` enforces that owner in Hadoop 3.3.4, 3.4.2, and 3.5.0. For reproduction, launch the bundled loader as an application user with `java.io.tmpdir` below such a NodeManager-owned ancestor and no installed libcomet on `java.library.path`. The new loop deterministically rejects the NodeManager directory. A JDK 17 probe of the exact base/head loader sources independently reproduced the same foreign-owner rejection in this sandbox: base reported `loaded=true` an d reused its library, while head threw `IllegalStateException: Cannot unpack libcomet` caused by `IOException: Unsafe temporary directory for libcomet: /tmp`. Both passed using `java.library.path`. Logs: `/tmp/comet-6987-current-review/base-bundled.log` and `/tmp/comet-6987-current-review/head-bundled.log`. This was a bounded loader test, not a live YARN deployment. -- 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]
