From fe48be4bea98bce3462541fe5a892ba828142e1b Mon Sep 17 00:00:00 2001 From: Juri Leino Date: Thu, 23 May 2024 23:15:05 +0200 Subject: [PATCH 1/4] [bugfix] do not set permissions on temporary document When XML resources are modified using an effective group because the setGID bit is set on the resource NativeBroker.defragXMLResource might throw a PermissionDeniedException. With this patch applied, this can no longer happen as the call to DocumentImpl#copyOf is removed in defragXMLresource. The temporary document is just a container to realign nodes on one BTree page and is destroyed within the method. --- .../src/main/java/org/exist/storage/NativeBroker.java | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/exist-core/src/main/java/org/exist/storage/NativeBroker.java b/exist-core/src/main/java/org/exist/storage/NativeBroker.java index 0ae299b738b..ef14244eaf3 100644 --- a/exist-core/src/main/java/org/exist/storage/NativeBroker.java +++ b/exist-core/src/main/java/org/exist/storage/NativeBroker.java @@ -3171,9 +3171,9 @@ public Object start() { return null; } }.run(); - // create a copy of the old doc to copy the nodes into it + // Create a copy of the old document to copy the nodes into it. + // tempDoc serves as a storage container for the child nodes. final DocumentImpl tempDoc = new DocumentImpl(null, doc.getDocId(), doc); - tempDoc.copyOf(this, doc, doc); final StreamListener listener = getIndexController().getStreamListener(doc, ReindexMode.STORE); // copy the nodes final NodeList nodes = doc.getChildNodes(); @@ -3198,15 +3198,17 @@ public Object start() { return null; } }.run(); + // assign new child nodes from the temporary document doc.copyChildren(tempDoc); doc.setSplitCount(0); doc.setPageCount(tempDoc.getPageCount()); + // store defragmented document storeXMLResource(transaction, doc); closeDocument(); if (LOG.isDebugEnabled()) { LOG.debug("Defragmentation took {} ms.", (System.currentTimeMillis() - start)); } - } catch(final PermissionDeniedException | IOException e) { + } catch(final IOException e) { LOG.error(e); } } From e6edd242b09eec1ff80c7cb07e8e8eb7f33edea5 Mon Sep 17 00:00:00 2001 From: Juri Leino Date: Thu, 23 May 2024 23:15:59 +0200 Subject: [PATCH 2/4] [test] address code review in UpdateInsertTriggersDefrag --- .../update/UpdateInsertTriggersDefrag.java | 46 ++++++++----------- 1 file changed, 18 insertions(+), 28 deletions(-) diff --git a/exist-core/src/test/java/org/exist/xquery/update/UpdateInsertTriggersDefrag.java b/exist-core/src/test/java/org/exist/xquery/update/UpdateInsertTriggersDefrag.java index 7c486f08999..fbba87382a4 100644 --- a/exist-core/src/test/java/org/exist/xquery/update/UpdateInsertTriggersDefrag.java +++ b/exist-core/src/test/java/org/exist/xquery/update/UpdateInsertTriggersDefrag.java @@ -21,66 +21,56 @@ */ package org.exist.xquery.update; -import org.exist.storage.DBBroker; import org.exist.test.ExistXmldbEmbeddedServer; -import org.exist.test.TestConstants; import org.junit.After; import org.junit.Before; import org.junit.ClassRule; import org.junit.Test; import org.xmldb.api.base.Collection; import org.xmldb.api.base.ResourceSet; -import org.xmldb.api.base.XMLDBException; import org.xmldb.api.modules.CollectionManagementService; import org.xmldb.api.modules.XMLResource; -import org.xmldb.api.modules.XPathQueryService; import org.xmldb.api.modules.XQueryService; +import static org.exist.storage.DBBroker.PROPERTY_XUPDATE_FRAGMENTATION_FACTOR; +import static org.exist.test.TestConstants.TEST_COLLECTION_URI; +import static org.exist.test.TestConstants.TEST_XML_URI; import static org.exist.util.PropertiesBuilder.propertiesBuilder; import static org.junit.Assert.assertEquals; public class UpdateInsertTriggersDefrag { @ClassRule - public static final ExistXmldbEmbeddedServer exist = new ExistXmldbEmbeddedServer(false, true, true, propertiesBuilder().put(DBBroker.PROPERTY_XUPDATE_FRAGMENTATION_FACTOR, -1).build()); - final String path = TestConstants.TEST_COLLECTION_URI + "/" + TestConstants.TEST_XML_URI.toString(); - final String xml = "initial"; - Collection testCollection; - XQueryService queryService; - CollectionManagementService collectionService; - - /** - * stores XML String and get Query Service - * - * @param documentName to be stored in the DB - * @param content to be stored in the DB - * @throws XMLDBException if an error occurs storing the document - */ - private void storeXML(final String documentName, final String content) throws XMLDBException { - final XMLResource doc = testCollection.createResource(documentName, XMLResource.class); - doc.setContent(content); - testCollection.storeResource(doc); - } + public static final ExistXmldbEmbeddedServer exist = new ExistXmldbEmbeddedServer(false, true, true, + propertiesBuilder().put(PROPERTY_XUPDATE_FRAGMENTATION_FACTOR, -1).build()); + private final String path = TEST_COLLECTION_URI + "/" + TEST_XML_URI.toString(); + private Collection testCollection; + private CollectionManagementService collectionService; @Before public void setUp() throws Exception { collectionService = exist.getRoot().getService(CollectionManagementService.class); - testCollection = collectionService.createCollection(TestConstants.TEST_COLLECTION_URI.toString()); - queryService = (XQueryService) testCollection.getService(XPathQueryService.class); - storeXML(TestConstants.TEST_XML_URI.toString(), xml); + testCollection = collectionService.createCollection(TEST_COLLECTION_URI.lastSegment().toString()); + final XMLResource doc = testCollection.createResource(TEST_XML_URI.toString(), XMLResource.class); + + doc.setContent("initial"); + testCollection.storeResource(doc); } @After public void tearDown() throws Exception { collectionService.removeCollection(testCollection.getName()); + testCollection.close(); } @Test public void triggerDefragAfterUpdate() throws Exception { + final XQueryService queryService = testCollection.getService(XQueryService.class); + final String update = "update insert new node into doc('" + path + "')//list"; - final ResourceSet updateResult = queryService.queryResource(TestConstants.TEST_XML_URI.toString(), update); + final ResourceSet updateResult = queryService.queryResource(path, update); assertEquals("Update expression returns an empty sequence", 0, updateResult.getSize()); - final ResourceSet itemResult = queryService.queryResource(TestConstants.TEST_XML_URI.toString(), "//item"); + final ResourceSet itemResult = queryService.queryResource(path, "//item"); assertEquals("Both items are returned", 2, itemResult.getSize()); } From cf0ec0f323329398fb05f21323239a2ad1569550 Mon Sep 17 00:00:00 2001 From: Juri Leino Date: Fri, 24 May 2024 11:21:44 +0200 Subject: [PATCH 3/4] [bugfix] revert to previous constructor call for tempDoc in defragXMLResource Testing has shown that the previous call to construct tempDoc was actually the correct one, as it is using the default file permissions of the pool. That is key for situations in which the subject initiating the defragmentation is not the _owner_ of the document and only belongs to the group with write permission. --- exist-core/src/main/java/org/exist/storage/NativeBroker.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/exist-core/src/main/java/org/exist/storage/NativeBroker.java b/exist-core/src/main/java/org/exist/storage/NativeBroker.java index ef14244eaf3..0d4f5d48a68 100644 --- a/exist-core/src/main/java/org/exist/storage/NativeBroker.java +++ b/exist-core/src/main/java/org/exist/storage/NativeBroker.java @@ -3173,7 +3173,7 @@ public Object start() { }.run(); // Create a copy of the old document to copy the nodes into it. // tempDoc serves as a storage container for the child nodes. - final DocumentImpl tempDoc = new DocumentImpl(null, doc.getDocId(), doc); + final DocumentImpl tempDoc = new DocumentImpl(null, pool, doc.getCollection(), doc.getDocId(), doc.getFileURI()); final StreamListener listener = getIndexController().getStreamListener(doc, ReindexMode.STORE); // copy the nodes final NodeList nodes = doc.getChildNodes(); From 6d21c25d042b6f607473c9e5474af171d042841f Mon Sep 17 00:00:00 2001 From: Dannes Wessels Date: Fri, 11 Oct 2024 18:45:39 +0200 Subject: [PATCH 4/4] Remove resource leak --- .../exist/xquery/update/UpdateInsertTriggersDefrag.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/exist-core/src/test/java/org/exist/xquery/update/UpdateInsertTriggersDefrag.java b/exist-core/src/test/java/org/exist/xquery/update/UpdateInsertTriggersDefrag.java index fbba87382a4..f62d997d39e 100644 --- a/exist-core/src/test/java/org/exist/xquery/update/UpdateInsertTriggersDefrag.java +++ b/exist-core/src/test/java/org/exist/xquery/update/UpdateInsertTriggersDefrag.java @@ -50,10 +50,11 @@ public class UpdateInsertTriggersDefrag { public void setUp() throws Exception { collectionService = exist.getRoot().getService(CollectionManagementService.class); testCollection = collectionService.createCollection(TEST_COLLECTION_URI.lastSegment().toString()); - final XMLResource doc = testCollection.createResource(TEST_XML_URI.toString(), XMLResource.class); - doc.setContent("initial"); - testCollection.storeResource(doc); + try (final XMLResource doc = testCollection.createResource(TEST_XML_URI.toString(), XMLResource.class)) { + doc.setContent("initial"); + testCollection.storeResource(doc); + } } @After