From 23d63bf094ff20fc55caa37f42ce7420d868a0e2 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Wed, 19 Aug 2026 14:43:21 +0300 Subject: [PATCH] [#874] Grant the offline tools a read-only JDBC transaction instead of refusing it export-ldif, verify-index and backendstat open a root container of their own, in READ_ONLY mode, whenever the backend is not already open - a stopped server or a disabled backend. RootContainer.open() asks the storage for a write transaction even in that mode, since that is where it opens the compressed schema and the entry containers, so JDBCStorage refusing to construct one failed all three tools before they read anything. The mode check moves from the constructor of WriteableTransactionTransactionImpl to the mutating operations - openTree(..., true), clearTree, deleteTree, put, update and delete. That is the shape the other three storages of this server already have: PDBStorage.ReadOnlyStorageImpl, JEStorage.ReadOnlyTransactionImpl and CASStorage.checkReadOnly(). The mode is captured once per transaction, since ImporterImpl reopens the storage READ_WRITE under its caller, and it also drives isReadOnly, so a cursor such a transaction opens refuses delete() as well. VLVIndex upgraded an untrusted index of an empty backend to trusted from its constructor, regardless of the access mode. That write moves to afterOpen(txn, createOnDemand), guarded exactly as DefaultIndex.afterOpen() already guards its own: it would otherwise fail the same three tools on an empty backend carrying a VLV index, on PDB and JE too. testReadOnly() caught none of this - it expects a ReadOnlyStorageException, and a storage that fails the open throws one too, from RootContainer.open() rather than from the write it means to be checking. testOfflineToolsOpenBackendReadOnly now runs export-ldif and verify-index against a closed backend for every pluggable backend, and testReadOnlyTransactionReadsButRefusesWrites asserts that the JDBC read-only transaction serves openTree(..., false), read, openCursor and getRecordCount while refusing every mutation. --- .../server/backends/jdbc/JDBCStorage.java | 29 ++++++- .../server/backends/pluggable/VLVIndex.java | 23 ++++-- .../opends/server/backends/jdbc/TestCase.java | 81 +++++++++++++++++++ .../PluggableBackendImplTestCase.java | 34 ++++++++ 4 files changed, 157 insertions(+), 10 deletions(-) diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java index d1d8055314..c94088c361 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java @@ -1043,6 +1043,16 @@ public long getRecordCount(TreeName treeName) { } } } + /** + * A transaction able to write, unless the storage was opened read-only: then it may open an existing tree and + * read it, and every mutating operation throws {@link ReadOnlyStorageException} instead. + *

+ * The mode is checked per operation rather than refused here, because {@code RootContainer.open(AccessMode)} + * asks for a write transaction even in read-only mode - that is where it opens the compressed schema and the + * entry containers - so refusing to hand one out failed the offline {@code export-ldif}, {@code verify-index} + * and {@code backendstat} before they read anything (#874). Both other storages of this server already have + * this shape: {@code PDBStorage.ReadOnlyStorageImpl} and {@code CASStorage.TransactionImpl.checkReadOnly()}. + */ private final class WriteableTransactionTransactionImpl extends ReadableTransactionImpl implements WriteableTransaction { // Shared by every table this transaction stamps: opening a backend opens all its trees, @@ -1052,10 +1062,17 @@ private final class WriteableTransactionTransactionImpl extends ReadableTransact public WriteableTransactionTransactionImpl(Connection con) { super(con); - if (!accessMode.isWriteable()) { + //captured once rather than read per operation: the access mode of the storage is mutable state - + //ImporterImpl reopens the storage READ_WRITE under its caller - and a transaction has to keep the mode + //it was created with. It also drives isReadOnly, so that a cursor this transaction opens refuses + //delete() as well. + isReadOnly = !accessMode.isWriteable(); + } + + void checkReadOnly() { + if (isReadOnly) { throw new ReadOnlyStorageException(); } - isReadOnly = false; } boolean isExistsTable(TreeName treeName) { @@ -1096,6 +1113,7 @@ String getTableDialect() { @Override public void openTree(TreeName treeName, boolean createOnDemand) { if (createOnDemand) { + checkReadOnly(); if (!isExistsTable(treeName)) { try (final PreparedStatement statement=con.prepareStatement("create table "+getTableName(treeName)+" ("+getTableDialect()+")")){ execute(statement); @@ -1158,6 +1176,7 @@ boolean isExistsIndex(String tableName, String indexName) throws SQLException { } public void clearTree(TreeName treeName) { + checkReadOnly(); try (final PreparedStatement statement=con.prepareStatement("delete from "+getTableName(treeName))){ execute(statement); con.commit(); @@ -1168,6 +1187,7 @@ public void clearTree(TreeName treeName) { @Override public void deleteTree(TreeName treeName) { + checkReadOnly(); if (isExistsTable(treeName)) { try (final PreparedStatement statement = con.prepareStatement("drop table " + getTableName(treeName))) { execute(statement); @@ -1183,6 +1203,7 @@ public void deleteTree(TreeName treeName) { @Override public void put(TreeName treeName, ByteSequence key, ByteSequence value) { + checkReadOnly(); try { upsert(treeName, key, value); } catch (SQLException e) { @@ -1250,6 +1271,9 @@ boolean update(TreeName treeName, ByteSequence key, ByteSequence value) throws S @Override public boolean update(TreeName treeName, ByteSequence key, UpdateFunction f) { + //checked before the read, so that a read-only transaction reports the mode rather than the value it + //computed being equal to the stored one + checkReadOnly(); final ByteString oldValue=read(treeName,key); final ByteSequence newValue=f.computeNewValue(oldValue); if (Objects.equals(newValue, oldValue)) @@ -1266,6 +1290,7 @@ public boolean update(TreeName treeName, ByteSequence key, UpdateFunction f) { @Override public boolean delete(TreeName treeName, ByteSequence key) { + checkReadOnly(); try (final PreparedStatement statement=con.prepareStatement("delete from "+getTableName(treeName)+" where h="+hashParam(con)+" and k=?")){ statement.setString(1,key2hash.get(ByteBuffer.wrap(key.toByteArray()))); statement.setBytes(2,real2db(key.toByteArray())); diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/VLVIndex.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/VLVIndex.java index 757aa99e16..ca84641b19 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/VLVIndex.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/VLVIndex.java @@ -105,6 +105,7 @@ class VLVIndex extends AbstractTree implements ConfigurationChangeListener cursor = txn.openCursor(tree)) { + assertTrue(cursor.next()); + assertEquals(cursor.getKey(), key(0)); + try { + cursor.delete(); + fail("delete() through a cursor of a read-only transaction must fail"); + } catch (UnsupportedOperationException expected) {} + } + + assertReadOnly("openTree(createOnDemand)", () -> txn.openTree(absent, true)); + assertReadOnly("put", () -> txn.put(tree, key(2), value(2))); + assertReadOnly("update", () -> txn.update(tree, key(0), old -> value(3))); + assertReadOnly("delete", () -> txn.delete(tree, key(0))); + assertReadOnly("deleteTree", () -> txn.deleteTree(tree)); + } + }); + + // nothing above reached the database + storage.close(); + storage.open(AccessMode.READ_WRITE); + storage.read(new ReadOperation() { + @Override + public Void run(ReadableTransaction txn) throws Exception { + assertEquals(txn.getRecordCount(tree), 2); + assertEquals(txn.read(tree, key(0)), value(0)); + return null; + } + }); + } finally { + try { + storage.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + txn.deleteTree(tree); + } + }); + } catch (Exception ignored) {} + storage.close(); + } + } + + private static void assertReadOnly(String operation, Runnable mutation) { + try { + mutation.run(); + fail(operation + " must fail on a read-only storage"); + } catch (ReadOnlyStorageException expected) {} + } + /** Buffer-served repositioning relies on the database collating keys in unsigned byte order. */ @Test public void testCursorKeyOrderIsUnsigned() throws Exception { diff --git a/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/PluggableBackendImplTestCase.java b/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/PluggableBackendImplTestCase.java index eef3190788..6706f7b402 100644 --- a/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/PluggableBackendImplTestCase.java +++ b/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/PluggableBackendImplTestCase.java @@ -1251,6 +1251,40 @@ public void run(WriteableTransaction txn) throws Exception } } + /** + * export-ldif, verify-index and backendstat open a root container of their own, in READ_ONLY mode, when the + * backend is not already open - a stopped server or a disabled backend. RootContainer.open() asks the storage + * for a write transaction even in that mode, since that is where it opens the compressed schema and the entry + * containers, so a storage refusing to hand one out fails the three tools before they read anything (#874). + *

+ * testReadOnly() above does not cover this: it expects a ReadOnlyStorageException and a storage that fails the + * open throws one too, from RootContainer.open() rather than from the write it is meant to be checking. + */ + @Test + public void testOfflineToolsOpenBackendReadOnly() throws Exception + { + // Put the backend offline, so that the tools open a read-only root container of their own + backend.finalizeBackend(); + try + { + final ByteArrayOutputStream exported = new ByteArrayOutputStream(); + try (final LDIFExportConfig exportConfig = new LDIFExportConfig(exported)) + { + backend.exportLDIF(exportConfig); + } + assertThat(exported.toString(StandardCharsets.UTF_8.name())).contains(testBaseDN.toString()); + + final VerifyConfig verifyConfig = new VerifyConfig(); + verifyConfig.setBaseDN(testBaseDN); + verifyConfig.addCompleteIndex("dn2id"); + assertThat(backend.verifyBackend(verifyConfig)).isEqualTo(0); + } + finally + { + backend.openBackend(); + } + } + @Test public void test_issue_496() throws Exception { int resultCode = TestCaseUtils.applyModifications(true,