andygrove commented on code in PR #6987: URL: https://github.com/apache/datafusion-comet/pull/6987#discussion_r4283431419
########## 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( + root, + DIRECTORY_PREFIX, + PosixFilePermissions.asFileAttribute(PosixFilePermissions.fromString("rwx------"))); + } catch (IOException e) { + failure.addSuppressed(e); + } + } + throw failure; + } + + 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")); + boolean sharedWritable = (mode & 0022) != 0; + boolean sticky = (mode & 01000) != 0; + if (!trustedOwner || (sharedWritable && !sticky)) { Review Comment: This also rejects a plain world-writable temp directory without the sticky bit. The case I am worried about is Kubernetes, where an `emptyDir` is created with mode `0777` and no sticky bit. It is common to mount one at `/tmp` when `readOnlyRootFilesystem` is set, and then `java.io.tmpdir`, `/tmp` and `/var/tmp` all fail this check (the last because the root filesystem is read-only), so no root is usable. The base code loads fine there because `File.createTempFile` has no such requirement. I compiled `NativeLibrary` from this head and ran `extract` against a single root with mode `0777`. It threw `IOException: No safe temporary directory for libcomet` with `Unsafe temporary directory for libcomet` suppressed. With mode `1777` it succeeded. `NativeBase` turns that into `Cannot unpack libcomet`, so Comet silently falls back to Spark by default, or fails startup with `spark.comet.nativeLoadRequired=true`. I could not run a real pod here, so the `emptyDir` mode is from the Kubernetes docs and not something I observed. Could the ancestor and sticky checks be dropped in favor of the private `0700` directory plus `CREATE_NEW` in `java.io.tmpdir`, with a property to relocate it, as comphead suggested? If the rename-in-a-non-sticky-parent threat is worth keeping, should that case log a warning and still load rather than refuse, so a deployment that worked on the release does not lose Comet? -- 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]
