Skip to content

Commit 89ade4f

Browse files
authored
[#10741] fix(hive): Fix keytab symlink TOCTOU race in FetchFileUtils (#10742)
### What changes were proposed in this pull request? Fix the `file:` scheme handler in `FetchFileUtils` to synchronize symlink creation and avoid concurrent races. Replace the non-atomic `exists() + delete()` in `KerberosClient` with `Files.deleteIfExists()`. Remove the unused duplicate `FetchFileUtils` in `catalog-hive` and add comprehensive tests in `hive-metastore-common`. ### Why are the changes needed? `Files.createSymbolicLink()` throws `FileAlreadyExistsException` when multiple threads concurrently fetch the same keytab path, such as when the search event listener triggers cascade sync after catalog creation. Fix: #10741 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? - added unit coverage in `TestFetchFileUtils` including concurrent, idempotent, and target-replacement scenarios
1 parent 2f53328 commit 89ade4f

5 files changed

Lines changed: 162 additions & 167 deletions

File tree

catalogs/catalog-hive/src/main/java/org/apache/gravitino/catalog/hive/FetchFileUtils.java

Lines changed: 0 additions & 65 deletions
This file was deleted.

catalogs/catalog-hive/src/test/java/org/apache/gravitino/catalog/hive/TestFetchFileUtils.java

Lines changed: 0 additions & 95 deletions
This file was deleted.

catalogs/hive-metastore-common/src/main/java/org/apache/gravitino/hive/kerberos/FetchFileUtils.java

Lines changed: 42 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,23 +19,45 @@
1919
package org.apache.gravitino.hive.kerberos;
2020

2121
import java.io.File;
22+
import java.io.IOException;
2223
import java.net.URI;
2324
import java.net.URISyntaxException;
2425
import java.nio.file.Files;
26+
import java.nio.file.StandardCopyOption;
27+
import java.util.Optional;
28+
import java.util.concurrent.ConcurrentHashMap;
2529
import org.apache.commons.io.FileUtils;
2630
import org.apache.hadoop.conf.Configuration;
2731
import org.apache.hadoop.fs.FileSystem;
2832
import org.apache.hadoop.fs.Path;
2933

3034
public class FetchFileUtils {
3135

36+
/**
37+
* Per-destination lock map used to serialize concurrent symlink creation for the same keytab
38+
* file. Keyed by the normalized absolute destination path string to avoid races caused by
39+
* different path spellings referring to the same file. Entries are removed when the corresponding
40+
* {@link KerberosClient} is closed, so the map size is bounded by the number of live catalogs.
41+
*/
42+
private static final ConcurrentHashMap<String, Object> SYMLINK_LOCKS = new ConcurrentHashMap<>();
43+
3244
private FetchFileUtils() {}
3345

46+
/**
47+
* Removes the per-destination lock entry for the given file. Should be called when the keytab
48+
* file is deleted (e.g., on {@link KerberosClient#close()}) to prevent unbounded map growth.
49+
*
50+
* @param destFile the keytab destination file whose lock entry should be removed
51+
*/
52+
static void removeLock(File destFile) {
53+
SYMLINK_LOCKS.remove(destFile.toPath().toAbsolutePath().normalize().toString());
54+
}
55+
3456
public static void fetchFileFromUri(
35-
String fileUri, File destFile, int timeout, Configuration conf) throws java.io.IOException {
57+
String fileUri, File destFile, int timeout, Configuration conf) throws IOException {
3658
try {
3759
URI uri = new URI(fileUri);
38-
String scheme = java.util.Optional.ofNullable(uri.getScheme()).orElse("file");
60+
String scheme = Optional.ofNullable(uri.getScheme()).orElse("file");
3961

4062
switch (scheme) {
4163
case "http":
@@ -45,7 +67,24 @@ public static void fetchFileFromUri(
4567
break;
4668

4769
case "file":
48-
Files.createSymbolicLink(destFile.toPath(), new File(uri.getPath()).toPath());
70+
var srcPath = new File(uri.getPath()).toPath().normalize();
71+
var destPath = destFile.toPath().toAbsolutePath().normalize();
72+
Object lock = SYMLINK_LOCKS.computeIfAbsent(destPath.toString(), k -> new Object());
73+
synchronized (lock) {
74+
// Skip if the symlink already points to the correct target.
75+
if (Files.isSymbolicLink(destPath)
76+
&& Files.readSymbolicLink(destPath).normalize().equals(srcPath)) {
77+
break;
78+
}
79+
// Replace via a temporary symlink + rename to minimize the window where the
80+
// keytab path is absent (which could cause loginUserFromKeytab to fail).
81+
// REPLACE_EXISTING is used here; on common local filesystems (ext4, xfs, APFS)
82+
// a same-directory rename is effectively atomic at the OS level.
83+
var tmpPath = destPath.resolveSibling(destPath.getFileName() + ".symlink.tmp");
84+
Files.deleteIfExists(tmpPath);
85+
Files.createSymbolicLink(tmpPath, srcPath);
86+
Files.move(tmpPath, destPath, StandardCopyOption.REPLACE_EXISTING);
87+
}
4988
break;
5089

5190
case "hdfs":

catalogs/hive-metastore-common/src/main/java/org/apache/gravitino/hive/kerberos/KerberosClient.java

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -172,10 +172,6 @@ public File saveKeyTabFileFromUri(String path) throws IOException {
172172
keytabsDir.mkdir();
173173
}
174174
File keytabFile = new File(path);
175-
if (keytabFile.exists() && !keytabFile.delete()) {
176-
throw new IllegalStateException(
177-
String.format("Fail to delete keytab file %s", keytabFile.getAbsolutePath()));
178-
}
179175
int fetchKeytabFileTimeout = kerberosConfig.getFetchTimeoutSec();
180176
FetchFileUtils.fetchFileFromUri(keyTabUri, keytabFile, fetchKeytabFileTimeout, hadoopConf);
181177
return keytabFile;
@@ -193,6 +189,7 @@ public void close() {
193189
}
194190

195191
Files.deleteIfExists(Paths.get(keytabFilePath));
192+
FetchFileUtils.removeLock(new File(keytabFilePath));
196193
} catch (IOException e) {
197194
LOG.warn("Failed to delete keytab file: {}", keytabFilePath, e);
198195
}
Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
package org.apache.gravitino.hive.kerberos;
20+
21+
import java.io.File;
22+
import java.nio.file.Files;
23+
import java.util.ArrayList;
24+
import java.util.List;
25+
import java.util.concurrent.CyclicBarrier;
26+
import java.util.concurrent.ExecutorService;
27+
import java.util.concurrent.Executors;
28+
import java.util.concurrent.Future;
29+
import java.util.concurrent.TimeUnit;
30+
import org.apache.hadoop.conf.Configuration;
31+
import org.junit.jupiter.api.Assertions;
32+
import org.junit.jupiter.api.Test;
33+
import org.junit.jupiter.api.io.TempDir;
34+
35+
public class TestFetchFileUtils {
36+
37+
@TempDir File tempDir;
38+
39+
@Test
40+
public void testLinkLocalFile() throws Exception {
41+
File srcFile = new File(tempDir, "source");
42+
Assertions.assertTrue(srcFile.createNewFile());
43+
File destFile = new File(tempDir, "dest");
44+
45+
FetchFileUtils.fetchFileFromUri(srcFile.toURI().toString(), destFile, 10, new Configuration());
46+
Assertions.assertTrue(Files.isSymbolicLink(destFile.toPath()));
47+
Assertions.assertEquals(srcFile.toPath(), Files.readSymbolicLink(destFile.toPath()));
48+
}
49+
50+
@Test
51+
public void testConcurrentSymlinkCreation() throws Exception {
52+
File srcFile = new File(tempDir, "source_concurrent");
53+
Assertions.assertTrue(srcFile.createNewFile());
54+
File destFile = new File(tempDir, "dest_concurrent");
55+
56+
int threadCount = 10;
57+
CyclicBarrier barrier = new CyclicBarrier(threadCount);
58+
ExecutorService executor = Executors.newFixedThreadPool(threadCount);
59+
List<Future<?>> futures = new ArrayList<>();
60+
try {
61+
for (int i = 0; i < threadCount; i++) {
62+
futures.add(
63+
executor.submit(
64+
() -> {
65+
try {
66+
barrier.await(30, TimeUnit.SECONDS);
67+
FetchFileUtils.fetchFileFromUri(
68+
srcFile.toURI().toString(), destFile, 10, new Configuration());
69+
} catch (Exception e) {
70+
throw new RuntimeException(e);
71+
}
72+
}));
73+
}
74+
for (Future<?> future : futures) {
75+
future.get(30, TimeUnit.SECONDS);
76+
}
77+
} finally {
78+
executor.shutdownNow();
79+
}
80+
81+
Assertions.assertTrue(Files.isSymbolicLink(destFile.toPath()));
82+
Assertions.assertEquals(srcFile.toPath(), Files.readSymbolicLink(destFile.toPath()));
83+
}
84+
85+
@Test
86+
public void testIdempotentSymlinkCreation() throws Exception {
87+
File srcFile = new File(tempDir, "source_idempotent");
88+
Assertions.assertTrue(srcFile.createNewFile());
89+
File destFile = new File(tempDir, "dest_idempotent");
90+
91+
Configuration conf = new Configuration();
92+
String uri = srcFile.toURI().toString();
93+
94+
FetchFileUtils.fetchFileFromUri(uri, destFile, 10, conf);
95+
Assertions.assertTrue(Files.isSymbolicLink(destFile.toPath()));
96+
97+
// Second call to the same dest should succeed without error
98+
FetchFileUtils.fetchFileFromUri(uri, destFile, 10, conf);
99+
Assertions.assertTrue(Files.isSymbolicLink(destFile.toPath()));
100+
Assertions.assertEquals(srcFile.toPath(), Files.readSymbolicLink(destFile.toPath()));
101+
}
102+
103+
@Test
104+
public void testSymlinkReplacedWithDifferentTarget() throws Exception {
105+
File srcFileA = new File(tempDir, "source_a");
106+
File srcFileB = new File(tempDir, "source_b");
107+
Assertions.assertTrue(srcFileA.createNewFile());
108+
Assertions.assertTrue(srcFileB.createNewFile());
109+
File destFile = new File(tempDir, "dest_replace");
110+
111+
Configuration conf = new Configuration();
112+
113+
FetchFileUtils.fetchFileFromUri(srcFileA.toURI().toString(), destFile, 10, conf);
114+
Assertions.assertEquals(srcFileA.toPath(), Files.readSymbolicLink(destFile.toPath()));
115+
116+
FetchFileUtils.fetchFileFromUri(srcFileB.toURI().toString(), destFile, 10, conf);
117+
Assertions.assertEquals(srcFileB.toPath(), Files.readSymbolicLink(destFile.toPath()));
118+
}
119+
}

0 commit comments

Comments
 (0)