sunchao commented on code in PR #6987: URL: https://github.com/apache/datafusion-comet/pull/6987#discussion_r4272948709
########## spark/src/main/java/org/apache/comet/NativeLibrary.java: ########## @@ -0,0 +1,157 @@ +/* + * 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 { + // Spark on YARN puts java.io.tmpdir below a NodeManager-owned directory. That account can + // replace descendants, so use a separately validated system root if the preferred root is + // unsafe. + return extract( + resource, + libraryName, + Paths.get(System.getProperty("java.io.tmpdir")), + Paths.get("/tmp"), + Paths.get("/var/tmp")); + } + + static NativeLibrary extract(InputStream resource, String libraryName, Path... tempRoots) + throws IOException { + Path directory = createExtractionDirectory(tempRoots); + 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 Path createExtractionDirectory(Path[] tempRoots) throws IOException { + IOException failure = new IOException("No safe temporary directory for libcomet"); + for (Path tempRoot : tempRoots) { + try { + Path root = tempRoot.toRealPath(); + checkTempRoot(root); + return Files.createTempDirectory( Review Comment: [P2] Continue past fallback roots that cannot load native libraries. When Spark's YARN temporary directory has a NodeManager-owned ancestor and `/tmp` is mounted `noexec`, validation rejects the preferred directory but successfully creates the private directory under `/tmp`. This return commits to that candidate, so `System.load` subsequently throws `UnsatisfiedLinkError: failed to map segment from shared object` without trying an executable `/var/tmp`. The base loads successfully from the YARN directory in the same setup. Consequently, this PR disables Comet through the default fallback or aborts initialization with `spark.comet.nativeLoadRequired=true`, despite an available usable fallback. Could selection continue after filesystem-specific loading failures, using a freshly opened resource while retaining validation and cleanup? Evidence: Reproduced with exact base/head `NativeBase` sources, the head's unchanged `NativeLibrary`, JDK 17, and a minimal JNI fixture in an isolated Ubuntu 24.04 container. Set `/yarn` to foreign ownership with mode 0755 and `/yarn/app/tmp` to application ownership with mode 0700. Run with `-Djava.io.tmpdir=/yarn/app/tmp`, no installed library, and Docker `--tmpfs /tmp:rw,noexec,nosuid,mode=1777`. Base: `loaded=true`, `reloaded=true reused=true`. Head: `UnsatisfiedLinkError` from `/tmp/comet-native-.../libcomet.so`. The same head with `-Djava.io.tmpdir=/var/tmp` reports successful loading and reuse, proving the later candidate is usable. Both successful and failed extractions were cleaned up. Reproduction script and output: `/tmp/comet-6987-head-review-lapklgo2/noexec-confirm.sh` and `/tmp/comet-6987-head-review-lapklgo2/noexec-confirm.log`. Spark's checked YARN launcher sources impose the relevant container-relative temporary directory across all five supported versions. -- 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]
