bitflicker64 commented on code in PR #3309: URL: https://github.com/apache/hugegraph/pull/3309#discussion_r4246727331
########## hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/backend/store/rocksdb/RocksDBIteratorPoolTest.java: ########## @@ -0,0 +1,140 @@ +/* + * 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.hugegraph.backend.store.rocksdb; + +import java.nio.charset.StandardCharsets; + +import org.apache.hugegraph.backend.store.rocksdb.RocksDBIteratorPool.ReusedRocksIterator; +import org.apache.hugegraph.testutil.Assert; +import org.junit.After; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; +import org.rocksdb.ColumnFamilyDescriptor; +import org.rocksdb.ColumnFamilyHandle; +import org.rocksdb.Options; +import org.rocksdb.RocksDB; +import org.rocksdb.RocksIterator; + +public class RocksDBIteratorPoolTest { + + private static final String CF = "iter-pool-cf"; + + @Rule + public TemporaryFolder temporary = new TemporaryFolder(); + + private Options options; + private RocksDB rocksdb; + private ColumnFamilyHandle cfh; + private RocksDBIteratorPool pool; + + @Before + public void setup() throws Exception { + this.options = new Options().setCreateIfMissing(true); + this.rocksdb = RocksDB.open(this.options, this.temporary.newFolder().getAbsolutePath()); + this.cfh = this.rocksdb.createColumnFamily(new ColumnFamilyDescriptor(bytes(CF))); + this.rocksdb.put(this.cfh, bytes("k1"), bytes("v1")); + this.rocksdb.put(this.cfh, bytes("k2"), bytes("v2")); + this.pool = new RocksDBIteratorPool(this.rocksdb, this.cfh); + } + + @After + public void teardown() { + // Native objects must be released child-first: pool, CF, DB, options + try { + this.pool.close(); + } finally { + this.cfh.close(); + this.rocksdb.close(); + this.options.close(); Review Comment: Fixed in 4d989c05d: `teardown()` now null-checks the pool, CF handle, DB and options before closing each, still child-first. With a failure injected into `setup()`, the run reports that `IllegalStateException` and nothing else. `RocksDBIteratorPoolTest` passed alone twice (3/0); `unit-test` 1027/0/0/8. ########## hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/rocksdb/RocksDBStdSessionsLifecycleTest.java: ########## @@ -0,0 +1,336 @@ +/* + * 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.hugegraph.unit.rocksdb; + +import java.io.File; +import java.util.ArrayList; +import java.util.List; +import java.util.NoSuchElementException; + +import org.apache.commons.configuration2.PropertiesConfiguration; +import org.apache.hugegraph.backend.store.BackendEntry.BackendColumnIterator; +import org.apache.hugegraph.backend.store.rocksdb.RocksDBOptions; +import org.apache.hugegraph.backend.store.rocksdb.RocksDBSessions; +import org.apache.hugegraph.backend.store.rocksdb.RocksDBSessions.Session; +import org.apache.hugegraph.backend.store.rocksdb.RocksDBStdSessions; +import org.apache.hugegraph.config.HugeConfig; +import org.apache.hugegraph.config.OptionSpace; +import org.apache.hugegraph.exception.BackendException; +import org.apache.hugegraph.testutil.Assert; +import org.apache.hugegraph.unit.BaseUnitTest; +import org.apache.hugegraph.unit.FakeObjects; +import org.junit.After; +import org.junit.BeforeClass; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; +import org.rocksdb.RocksDBException; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; + +/** + * Lifecycle and negative-path tests for {@link RocksDBStdSessions}. Every test + * uses its own temporary directory so it never shares the fixed path used by + * {@link BaseRocksDBUnitTest}. + */ +public class RocksDBStdSessionsLifecycleTest extends BaseUnitTest { + + private static final String TABLE = "lifecycle-table"; + + @Rule + public TemporaryFolder temporary = new TemporaryFolder(); + + private final List<RocksDBSessions> opened = new ArrayList<>(); + + @BeforeClass + public static void registerRocksDBOptions() { + // Typed values in HugeConfig require the RocksDB options to be known + if (!OptionSpace.containKey(RocksDBOptions.NUM_LEVELS.name())) { + OptionSpace.register("rocksdb", RocksDBOptions.class.getName()); + } + } + + @After + public void closeOpenedSessions() { + for (RocksDBSessions sessions : this.opened) { + try { + // Closes this thread's session and, when it is the last one, the DB + sessions.close(); + } finally { + if (sessions.databaseOpened()) { + sessions.forceCloseRocksDB(); + } + } + } + this.opened.clear(); + } + + @Test + public void testOperationsOnUnopenedTableAreRejected() throws Exception { + RocksDBSessions sessions = this.open(this.newPath("unopened")); + Session session = sessions.session(); + byte[] key = getBytes("k"); + + Assert.assertThrows(BackendException.class, () -> { + session.put("missing", key, getBytes("v")); + }, e -> { + Assert.assertContains("Table 'missing' is not opened", e.getMessage()); + }); + Assert.assertFalse(session.hasChanges()); + + Assert.assertThrows(BackendException.class, () -> { + session.get("missing", key); + }, e -> { + Assert.assertContains("Table 'missing' is not opened", e.getMessage()); + }); + Assert.assertThrows(BackendException.class, () -> { + session.scan("missing"); + }, e -> { + Assert.assertContains("Table 'missing' is not opened", e.getMessage()); + }); + Assert.assertThrows(BackendException.class, () -> { + session.delete("missing", key); + }, e -> { + Assert.assertContains("Table 'missing' is not opened", e.getMessage()); + }); + Assert.assertThrows(BackendException.class, () -> { + session.keyRange("missing"); + }, e -> { + Assert.assertContains("Table 'missing' is not opened", e.getMessage()); + }); + + // The opened table is still fully usable after the rejected calls + session.put(TABLE, key, getBytes("v")); + Assert.assertEquals(1, (int) session.commit()); + Assert.assertEquals("v", getString(session.get(TABLE, key))); + } + + @Test + public void testCreateTableTwiceKeepsDataAndDropMissingTableIsNoop() throws Exception { + RocksDBSessions sessions = this.open(this.newPath("idempotent")); + Session session = sessions.session(); + session.put(TABLE, getBytes("k"), getBytes("v")); + session.commit(); + + // Re-creating an existing table must not truncate or replace it + sessions.createTable(TABLE); + Assert.assertEquals("v", getString(session.get(TABLE, getBytes("k")))); + + // Dropping a table that never existed leaves the opened tables intact + sessions.dropTable("never-created"); + Assert.assertTrue(sessions.existsTable(TABLE)); + Assert.assertFalse(sessions.existsTable("never-created")); + + sessions.dropTable(TABLE); + Assert.assertFalse(sessions.existsTable(TABLE)); + // Dropping again after the table is gone is still a no-op + sessions.dropTable(TABLE); + Assert.assertFalse(sessions.openedTables().contains(TABLE)); + } + + @Test + public void testRollbackDiscardsPendingWrites() throws Exception { + RocksDBSessions sessions = this.open(this.newPath("rollback")); + Session session = sessions.session(); + + Assert.assertEquals(0, (int) session.commit()); + session.put(TABLE, getBytes("k2"), getBytes("v2")); + Assert.assertEquals(1, (int) session.commit()); + + session.put(TABLE, getBytes("k1"), getBytes("v1")); + session.delete(TABLE, getBytes("k2")); + Assert.assertTrue(session.hasChanges()); + + session.rollback(); + Assert.assertFalse(session.hasChanges()); + Assert.assertEquals(0, (int) session.commit()); + // Neither the pending put nor the pending delete reached the DB + Assert.assertNull(session.get(TABLE, getBytes("k1"))); + Assert.assertEquals("v2", getString(session.get(TABLE, getBytes("k2")))); + } + + @Test + public void testCommittedDataSurvivesReopenButPendingWritesDoNot() throws Exception { + String path = this.newPath("reopen"); + RocksDBSessions sessions = this.open(path); + Session session = sessions.session(); + session.put(TABLE, getBytes("committed"), getBytes("v1")); + session.commit(); + session.put(TABLE, getBytes("pending"), getBytes("v2")); + Assert.assertTrue(session.hasChanges()); + + Assert.assertTrue(sessions.close()); + Assert.assertFalse(sessions.databaseOpened()); + + // Reopen without createTable(): existing column families are reloaded + RocksDBSessions reopened = this.openWithoutTable(path); + Assert.assertTrue(reopened.existsTable(TABLE)); + Session session2 = reopened.session(); + Assert.assertFalse(session2.hasChanges()); + Assert.assertEquals("v1", getString(session2.get(TABLE, getBytes("committed")))); + Assert.assertNull(session2.get(TABLE, getBytes("pending"))); + } + + @Test + public void testLiveDatabaseCannotBeOpenedTwiceUntilClosed() throws Exception { + String path = this.newPath("exclusive"); + RocksDBSessions sessions = this.open(path); + sessions.session().put(TABLE, getBytes("k"), getBytes("v")); + sessions.session().commit(); + + Assert.assertThrows(RocksDBException.class, () -> { + this.openWithoutTable(path); + }, e -> { + Assert.assertContains("lock", e.getMessage()); Review Comment: Keeping this one. The error comes from RocksDB's file-lock path, and every port includes "lock" in it: here it is "lock hold by current process" because the second open is in the same JVM. The check is what tells lock contention apart from any other open failure, which is the behaviour the test protects. Matching only the exception type would also accept unrelated `RocksDBException`s. ########## hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/rocksdb/RocksDBStdSessionsLifecycleTest.java: ########## @@ -0,0 +1,336 @@ +/* + * 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.hugegraph.unit.rocksdb; + +import java.io.File; +import java.util.ArrayList; +import java.util.List; +import java.util.NoSuchElementException; + +import org.apache.commons.configuration2.PropertiesConfiguration; +import org.apache.hugegraph.backend.store.BackendEntry.BackendColumnIterator; +import org.apache.hugegraph.backend.store.rocksdb.RocksDBOptions; +import org.apache.hugegraph.backend.store.rocksdb.RocksDBSessions; +import org.apache.hugegraph.backend.store.rocksdb.RocksDBSessions.Session; +import org.apache.hugegraph.backend.store.rocksdb.RocksDBStdSessions; +import org.apache.hugegraph.config.HugeConfig; +import org.apache.hugegraph.config.OptionSpace; +import org.apache.hugegraph.exception.BackendException; +import org.apache.hugegraph.testutil.Assert; +import org.apache.hugegraph.unit.BaseUnitTest; +import org.apache.hugegraph.unit.FakeObjects; +import org.junit.After; +import org.junit.BeforeClass; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; +import org.rocksdb.RocksDBException; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; + +/** + * Lifecycle and negative-path tests for {@link RocksDBStdSessions}. Every test + * uses its own temporary directory so it never shares the fixed path used by + * {@link BaseRocksDBUnitTest}. + */ +public class RocksDBStdSessionsLifecycleTest extends BaseUnitTest { + + private static final String TABLE = "lifecycle-table"; + + @Rule + public TemporaryFolder temporary = new TemporaryFolder(); + + private final List<RocksDBSessions> opened = new ArrayList<>(); + + @BeforeClass + public static void registerRocksDBOptions() { + // Typed values in HugeConfig require the RocksDB options to be known + if (!OptionSpace.containKey(RocksDBOptions.NUM_LEVELS.name())) { + OptionSpace.register("rocksdb", RocksDBOptions.class.getName()); + } + } + + @After + public void closeOpenedSessions() { + for (RocksDBSessions sessions : this.opened) { + try { + // Closes this thread's session and, when it is the last one, the DB + sessions.close(); + } finally { + if (sessions.databaseOpened()) { + sessions.forceCloseRocksDB(); + } + } + } + this.opened.clear(); + } + + @Test + public void testOperationsOnUnopenedTableAreRejected() throws Exception { + RocksDBSessions sessions = this.open(this.newPath("unopened")); + Session session = sessions.session(); + byte[] key = getBytes("k"); + + Assert.assertThrows(BackendException.class, () -> { + session.put("missing", key, getBytes("v")); + }, e -> { + Assert.assertContains("Table 'missing' is not opened", e.getMessage()); + }); + Assert.assertFalse(session.hasChanges()); + + Assert.assertThrows(BackendException.class, () -> { + session.get("missing", key); + }, e -> { + Assert.assertContains("Table 'missing' is not opened", e.getMessage()); + }); + Assert.assertThrows(BackendException.class, () -> { + session.scan("missing"); + }, e -> { + Assert.assertContains("Table 'missing' is not opened", e.getMessage()); + }); + Assert.assertThrows(BackendException.class, () -> { + session.delete("missing", key); + }, e -> { + Assert.assertContains("Table 'missing' is not opened", e.getMessage()); + }); + Assert.assertThrows(BackendException.class, () -> { + session.keyRange("missing"); + }, e -> { + Assert.assertContains("Table 'missing' is not opened", e.getMessage()); + }); + + // The opened table is still fully usable after the rejected calls + session.put(TABLE, key, getBytes("v")); + Assert.assertEquals(1, (int) session.commit()); + Assert.assertEquals("v", getString(session.get(TABLE, key))); + } + + @Test + public void testCreateTableTwiceKeepsDataAndDropMissingTableIsNoop() throws Exception { + RocksDBSessions sessions = this.open(this.newPath("idempotent")); + Session session = sessions.session(); + session.put(TABLE, getBytes("k"), getBytes("v")); + session.commit(); + + // Re-creating an existing table must not truncate or replace it + sessions.createTable(TABLE); + Assert.assertEquals("v", getString(session.get(TABLE, getBytes("k")))); + + // Dropping a table that never existed leaves the opened tables intact + sessions.dropTable("never-created"); + Assert.assertTrue(sessions.existsTable(TABLE)); + Assert.assertFalse(sessions.existsTable("never-created")); + + sessions.dropTable(TABLE); + Assert.assertFalse(sessions.existsTable(TABLE)); + // Dropping again after the table is gone is still a no-op + sessions.dropTable(TABLE); + Assert.assertFalse(sessions.openedTables().contains(TABLE)); + } + + @Test + public void testRollbackDiscardsPendingWrites() throws Exception { + RocksDBSessions sessions = this.open(this.newPath("rollback")); + Session session = sessions.session(); + + Assert.assertEquals(0, (int) session.commit()); + session.put(TABLE, getBytes("k2"), getBytes("v2")); + Assert.assertEquals(1, (int) session.commit()); + + session.put(TABLE, getBytes("k1"), getBytes("v1")); + session.delete(TABLE, getBytes("k2")); + Assert.assertTrue(session.hasChanges()); + + session.rollback(); + Assert.assertFalse(session.hasChanges()); + Assert.assertEquals(0, (int) session.commit()); + // Neither the pending put nor the pending delete reached the DB + Assert.assertNull(session.get(TABLE, getBytes("k1"))); + Assert.assertEquals("v2", getString(session.get(TABLE, getBytes("k2")))); + } + + @Test + public void testCommittedDataSurvivesReopenButPendingWritesDoNot() throws Exception { + String path = this.newPath("reopen"); + RocksDBSessions sessions = this.open(path); + Session session = sessions.session(); + session.put(TABLE, getBytes("committed"), getBytes("v1")); + session.commit(); + session.put(TABLE, getBytes("pending"), getBytes("v2")); + Assert.assertTrue(session.hasChanges()); + + Assert.assertTrue(sessions.close()); + Assert.assertFalse(sessions.databaseOpened()); + + // Reopen without createTable(): existing column families are reloaded + RocksDBSessions reopened = this.openWithoutTable(path); + Assert.assertTrue(reopened.existsTable(TABLE)); + Session session2 = reopened.session(); + Assert.assertFalse(session2.hasChanges()); + Assert.assertEquals("v1", getString(session2.get(TABLE, getBytes("committed")))); + Assert.assertNull(session2.get(TABLE, getBytes("pending"))); + } + + @Test + public void testLiveDatabaseCannotBeOpenedTwiceUntilClosed() throws Exception { + String path = this.newPath("exclusive"); + RocksDBSessions sessions = this.open(path); + sessions.session().put(TABLE, getBytes("k"), getBytes("v")); + sessions.session().commit(); + + Assert.assertThrows(RocksDBException.class, () -> { + this.openWithoutTable(path); + }, e -> { + Assert.assertContains("lock", e.getMessage()); + }); + // The failed open must not disturb the live instance + Assert.assertTrue(sessions.databaseOpened()); + Assert.assertEquals("v", getString(sessions.session().get(TABLE, getBytes("k")))); + + Assert.assertTrue(sessions.close()); + RocksDBSessions reopened = this.openWithoutTable(path); + Assert.assertEquals("v", getString(reopened.session().get(TABLE, getBytes("k")))); + } + + @Test + public void testClosedDatabaseRejectsTableAndSessionAccess() throws Exception { + String path = this.newPath("closed"); + RocksDBSessions sessions = this.open(path); + sessions.forceCloseRocksDB(); + Assert.assertFalse(sessions.databaseOpened()); + + Assert.assertThrows(IllegalStateException.class, () -> { + sessions.createTable("another-table"); + }, e -> { + Assert.assertContains("RocksDB has been closed", e.getMessage()); + }); + Assert.assertThrows(IllegalStateException.class, () -> { + sessions.dropTable(TABLE); + }, e -> { + Assert.assertContains("RocksDB has been closed", e.getMessage()); + }); + Assert.assertThrows(IllegalStateException.class, sessions::compactRange, e -> { + Assert.assertContains("RocksDB has been closed", e.getMessage()); + }); + Assert.assertThrows(IllegalStateException.class, sessions::session, e -> { + Assert.assertContains("session pool is closed", e.getMessage()); + }); + + // Force close must release the database lock for a later owner + RocksDBSessions reopened = this.openWithoutTable(path); + Assert.assertTrue(reopened.databaseOpened()); + Assert.assertEquals(ImmutableSet.of("default", TABLE), reopened.openedTables()); + } + + @Test + public void testScanIteratorCloseIsIdempotentAndStopsIteration() throws Exception { + RocksDBSessions sessions = this.open(this.newPath("scan-close")); + Session session = sessions.session(); + session.put(TABLE, getBytes("a1"), getBytes("1")); + session.put(TABLE, getBytes("a2"), getBytes("2")); + session.put(TABLE, getBytes("b1"), getBytes("3")); + session.commit(); + + BackendColumnIterator iter = session.scan(TABLE, getBytes("a")); + Assert.assertTrue(iter.hasNext()); + Assert.assertEquals("a1", getString(iter.next().name)); + Assert.assertEquals("a1", getString(iter.position())); + + // Closing early releases the native iterator; further reads end cleanly + iter.close(); + iter.close(); + Assert.assertFalse(iter.hasNext()); + Assert.assertThrows(NoSuchElementException.class, iter::next); + + // A fully consumed iterator clears its paging position + BackendColumnIterator all = session.scan(TABLE, getBytes("a")); + Assert.assertEquals("a1", getString(all.next().name)); + Assert.assertEquals("a2", getString(all.next().name)); + Assert.assertFalse(all.hasNext()); + Assert.assertNull(all.position()); + all.close(); + } + + @Test + public void testMismatchedLevelCompressionsRejectedAndLockReleased() throws Exception { + String path = this.newPath("bad-options"); + PropertiesConfiguration conf = new PropertiesConfiguration(); + conf.setProperty(RocksDBOptions.NUM_LEVELS.name(), "3"); + conf.setProperty(RocksDBOptions.LEVELS_COMPRESSIONS.name(), "[none, snappy]"); + HugeConfig badConfig = new HugeConfig(conf); + + Assert.assertThrows(IllegalArgumentException.class, () -> { + new RocksDBStdSessions(badConfig, "db", "store", path, path); + }, e -> { + Assert.assertContains(RocksDBOptions.LEVELS_COMPRESSIONS.name(), e.getMessage()); + Assert.assertContains("but got 2 != 3", e.getMessage()); + }); + + // The failed open must not leave the database lock behind + RocksDBSessions sessions = this.open(path); + Assert.assertTrue(sessions.databaseOpened()); + } + + @Test + public void testBuildSnapshotPathRequiresExistingSnapshot() throws Exception { + File parent = this.temporary.newFolder("graph"); + File dataDir = new File(parent, "rocksdb-data/g"); + Assert.assertTrue(dataDir.getParentFile().mkdirs()); + String dataPath = dataDir.getAbsolutePath(); + RocksDBSessions sessions = this.open(dataPath); + + File expected = new File(parent, "snapshot_rocksdb-data/g"); + Assert.assertThrows(IllegalArgumentException.class, () -> { + sessions.buildSnapshotPath("snapshot"); + }, e -> { + Assert.assertContains("doesn't exist", e.getMessage()); + Assert.assertContains(expected.getPath(), e.getMessage()); + }); + + Assert.assertTrue(expected.mkdirs()); + Assert.assertEquals(expected.getPath(), sessions.buildSnapshotPath("snapshot")); + } + + private String newPath(String name) throws Exception { + return new File(this.temporary.newFolder(name), "db").getAbsolutePath(); + } + + private RocksDBSessions open(String path) throws RocksDBException { + RocksDBSessions sessions = this.openWithoutTable(path); + sessions.createTable(TABLE); + return sessions; + } + + private RocksDBSessions openWithoutTable(String path) throws RocksDBException { + // Passing no CF names still reopens every column family found on disk + RocksDBSessions sessions = new RocksDBStdSessions(FakeObjects.newConfig(), "db", + "store", path, path, + ImmutableList.of()); + this.opened.add(sessions); + return sessions; + } + + private static byte[] getBytes(String str) { + return str.getBytes(); + } + + private static String getString(byte[] bytes) { + return bytes == null ? null : new String(bytes); + } Review Comment: Keeping the helpers as they are. They copy `BaseRocksDBUnitTest.getBytes/getString`, which the other RocksDB unit tests use, and every value passed through them is an ASCII literal ("k", "v1", "committed" and so on), so any platform charset gives the same bytes. Changing only this class would make it differ from its neighbours without changing any result. ########## hugegraph-store/hg-store-test/src/main/java/org/apache/hugegraph/store/rocksdb/ZipUtilsPathSafetyTest.java: ########## @@ -0,0 +1,156 @@ +/* + * 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.hugegraph.store.rocksdb; + +import static org.junit.Assert.assertArrayEquals; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; + +import java.io.File; +import java.io.FileOutputStream; +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.util.zip.CRC32; +import java.util.zip.ZipEntry; +import java.util.zip.ZipOutputStream; + +import org.apache.hugegraph.rocksdb.access.util.CRC64; +import org.apache.hugegraph.rocksdb.access.util.ZipUtils; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; + +/** + * Snapshot archives are produced and unpacked by two ZipUtils copies: the RocksDB one used for + * partition snapshots and the core one. Both must keep extracted files inside the target dir. + */ +public class ZipUtilsPathSafetyTest { + + @Rule + public TemporaryFolder folder = new TemporaryFolder(); + + private static void write(File file, String content) throws IOException { + Files.createDirectories(file.getParentFile().toPath()); + Files.write(file.toPath(), content.getBytes(StandardCharsets.UTF_8)); + } + + private static String read(File file) throws IOException { + return new String(Files.readAllBytes(file.toPath()), StandardCharsets.UTF_8); + } + + private File maliciousZip(String... entryNames) throws IOException { + File zip = this.folder.newFile("malicious.zip"); + try (ZipOutputStream zos = new ZipOutputStream(new FileOutputStream(zip))) { + zos.putNextEntry(new ZipEntry("snapshot/ok.sst")); + zos.write("ok".getBytes(StandardCharsets.UTF_8)); + zos.closeEntry(); + for (String name : entryNames) { + zos.putNextEntry(new ZipEntry(name)); + zos.write("pwned".getBytes(StandardCharsets.UTF_8)); + zos.closeEntry(); + } + } + return zip; + } + + @Test + public void testRoundTripKeepsNestedLayoutAndChecksumsAgree() throws IOException { + File root = this.folder.newFolder("root"); + write(new File(root, "snapshot/CURRENT"), "MANIFEST-000001"); + write(new File(root, "snapshot/sub/dir/000004.sst"), "sst-bytes"); + Files.createDirectories(new File(root, "snapshot/empty").toPath()); + File zip = new File(this.folder.getRoot(), "snapshot.zip"); + File out = this.folder.newFolder("out"); + + CRC64 written = new CRC64(); + ZipUtils.compress(root.getAbsolutePath(), "snapshot", zip.getAbsolutePath(), written); + CRC64 read = new CRC64(); + ZipUtils.decompress(zip.getAbsolutePath(), out.getAbsolutePath(), read); + + assertEquals("MANIFEST-000001", read(new File(out, "snapshot/CURRENT"))); + assertEquals("sst-bytes", read(new File(out, "snapshot/sub/dir/000004.sst"))); + // The checksum covers the whole archive on both sides, so it detects transfer damage + assertTrue(written.getValue() != 0L); + assertEquals(written.getValue(), read.getValue()); + + File coreOut = this.folder.newFolder("core-out"); + CRC32 coreRead = new CRC32(); + org.apache.hugegraph.store.util.ZipUtils.decompress(zip.getAbsolutePath(), + coreOut.getAbsolutePath(), coreRead); + assertArrayEquals(Files.readAllBytes(new File(out, "snapshot/sub/dir/000004.sst").toPath()), + Files.readAllBytes(new File(coreOut, "snapshot/sub/dir/000004.sst").toPath())); + } + + @Test + public void testRocksDbDecompressSkipsEntriesEscapingTheTargetDir() throws IOException { + File out = this.folder.newFolder("out"); + File zip = maliciousZip("../escaped.txt", "snapshot/../../escaped2.txt", "../out-evil/x.txt", + new File(this.folder.getRoot(), "absolute.txt").getAbsolutePath()); + + ZipUtils.decompress(zip.getAbsolutePath(), out.getAbsolutePath(), new CRC32()); + + assertEquals("ok", read(new File(out, "snapshot/ok.sst"))); + assertFalse(new File(this.folder.getRoot(), "escaped.txt").exists()); + assertFalse(new File(this.folder.getRoot(), "escaped2.txt").exists()); + assertFalse(new File(this.folder.getRoot(), "out-evil").exists()); + assertFalse(new File(this.folder.getRoot(), "absolute.txt").exists()); + } + + @Test + public void testCoreDecompressRejectsEntriesEscapingTheTargetDir() throws IOException { + File out = this.folder.newFolder("out"); + // "../out-evil" shares the textual prefix of the target dir and must still be rejected + for (String evil : new String[]{"../escaped.txt", "snapshot/../../escaped.txt", + "../out-evil/x.txt"}) { + File zip = maliciousZip(evil); + IOException e = assertThrows(IOException.class, () -> + org.apache.hugegraph.store.util.ZipUtils.decompress( + zip.getAbsolutePath(), out.getAbsolutePath(), new CRC32())); + assertTrue(e.getMessage(), e.getMessage().contains("outside of the target dir")); + assertFalse(new File(this.folder.getRoot(), "escaped.txt").exists()); + assertFalse(new File(this.folder.getRoot(), "out-evil").exists()); + assertTrue(zip.delete()); Review Comment: Keeping it. The delete is needed because `maliciousZip()` reuses one file name and `TemporaryFolder.newFile` fails if it already exists. The archive is written with try-with-resources, and `decompress` closes its handle. If the delete ever failed, it would point to a leaked handle, which is worth reporting. Store CI runs on Linux only. -- 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]
