This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 4b702743a743b1b61029692a87e2cab727cb0334 Author: opencode <[email protected]> AuthorDate: Fri Oct 9 10:47:41 2026 +0200 Make the configuration file swap in StoreFileMover crash-safe move() installed a newly stored configuration file with two renames: the previous file was first renamed to the timestamped backup name, then the new file was renamed to the active name. A crash between the two left the active server.xml or context.xml missing, and Tomcat could not start until an administrator renamed the backup back manually. Copy the previous file to the backup location instead of renaming it away, and install the new file with a single move which replaces the target atomically where the file system supports it. The active configuration file is now never absent during the swap and the restore path is no longer needed. --- .../catalina/storeconfig/LocalStrings.properties | 2 +- .../catalina/storeconfig/StoreFileMover.java | 55 +++++++--- .../catalina/storeconfig/TestStoreFileMover.java | 118 +++++++++++++++++++++ 3 files changed, 158 insertions(+), 17 deletions(-) diff --git a/java/org/apache/catalina/storeconfig/LocalStrings.properties b/java/org/apache/catalina/storeconfig/LocalStrings.properties index aabbe211ad..e207249494 100644 --- a/java/org/apache/catalina/storeconfig/LocalStrings.properties +++ b/java/org/apache/catalina/storeconfig/LocalStrings.properties @@ -47,10 +47,10 @@ storeConfigListener.registerError=Error registering StoreConfig MBean storeFactory.noDescriptor=Descriptor for element [{0}].[{1}] not configured +storeFileMover.copyError=Failed to copy the configuration file [{0}] to the backup location [{1}] storeFileMover.directoryCreationError=Cannot create directory [{0}] storeFileMover.null=Invalid null basename, filename or encoding storeFileMover.renameError=Cannot rename [{0}] to [{1}] -storeFileMover.restoreError=Rename [{0}] to [{1}] failed and restoring also failed xmlFormatPreserver.failed=Failed to preserve the original layout of the configuration file xmlFormatPreserver.unreadable=Failed to read the previous version of the configuration file [{0}] diff --git a/java/org/apache/catalina/storeconfig/StoreFileMover.java b/java/org/apache/catalina/storeconfig/StoreFileMover.java index 43180e314a..f0706a2423 100644 --- a/java/org/apache/catalina/storeconfig/StoreFileMover.java +++ b/java/org/apache/catalina/storeconfig/StoreFileMover.java @@ -21,6 +21,9 @@ import java.io.FileOutputStream; import java.io.IOException; import java.io.OutputStreamWriter; import java.io.PrintWriter; +import java.nio.file.AtomicMoveNotSupportedException; +import java.nio.file.Files; +import java.nio.file.StandardCopyOption; import java.sql.Timestamp; import org.apache.catalina.Globals; @@ -176,33 +179,53 @@ public class StoreFileMover { } /** - * Shuffle old->save and new->old. + * Install the new configuration file, saving the previous one as a backup when it exists. The backup is a copy and + * the active file is replaced by a single move, so the configuration file is never absent. * * @throws IOException a file operation error occurred */ public void move() throws IOException { - if (configOld.renameTo(configSave)) { - if (!configNew.renameTo(configOld)) { - if (!configSave.renameTo(configOld)) { - throw new IOException(sm.getString("storeFileMover.restoreError", configNew.getAbsolutePath(), - configOld.getAbsolutePath())); - } + if (configOld.exists()) { + // Copy the existing configuration file to the backup location rather + // than renaming it away. Replacing the configuration file with a move + // is atomic, so the active configuration file is never absent, even + // when the process dies before the replacement happens + try { + Files.copy(configOld.toPath(), configSave.toPath(), StandardCopyOption.COPY_ATTRIBUTES); + } catch (IOException ioe) { + throw new IOException(sm.getString("storeFileMover.copyError", configOld.getAbsolutePath(), + configSave.getAbsolutePath()), ioe); + } + try { + Files.move(configNew.toPath(), configOld.toPath(), StandardCopyOption.REPLACE_EXISTING, + StandardCopyOption.ATOMIC_MOVE); + } catch (AtomicMoveNotSupportedException amns) { + // Fall back to a plain replace when atomic moves are unsupported + moveWithReplace(); + } catch (IOException ioe) { throw new IOException(sm.getString("storeFileMover.renameError", configNew.getAbsolutePath(), - configOld.getAbsolutePath())); + configOld.getAbsolutePath()), ioe); } } else { - if (!configOld.exists()) { - if (!configNew.renameTo(configOld)) { - throw new IOException(sm.getString("storeFileMover.renameError", configNew.getAbsolutePath(), - configOld.getAbsolutePath())); - } - } else { - throw new IOException(sm.getString("storeFileMover.renameError", configOld.getAbsolutePath(), - configSave.getAbsolutePath())); + // Nothing to back up: install the new file directly + try { + Files.move(configNew.toPath(), configOld.toPath()); + } catch (IOException ioe) { + throw new IOException(sm.getString("storeFileMover.renameError", configNew.getAbsolutePath(), + configOld.getAbsolutePath()), ioe); } } } + private void moveWithReplace() throws IOException { + try { + Files.move(configNew.toPath(), configOld.toPath(), StandardCopyOption.REPLACE_EXISTING); + } catch (IOException ioe) { + throw new IOException(sm.getString("storeFileMover.renameError", configNew.getAbsolutePath(), + configOld.getAbsolutePath()), ioe); + } + } + /** * Open an output writer for the new configuration file. * diff --git a/test/org/apache/catalina/storeconfig/TestStoreFileMover.java b/test/org/apache/catalina/storeconfig/TestStoreFileMover.java new file mode 100644 index 0000000000..e09661bce5 --- /dev/null +++ b/test/org/apache/catalina/storeconfig/TestStoreFileMover.java @@ -0,0 +1,118 @@ +/* + * 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.catalina.storeconfig; + +import java.io.IOException; +import java.io.PrintWriter; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Comparator; +import java.util.stream.Stream; + +import org.junit.Assert; +import org.junit.Test; + +public class TestStoreFileMover { + + /** + * Verify the regular swap: the new content is installed, the old content is preserved in the backup and the + * intermediate file is consumed. + * + * @throws Exception if the test experiences an unexpected error + */ + @Test + public void testMoveWithBackup() throws Exception { + Path dir = Files.createTempDirectory("storeFileMover"); + try { + Files.writeString(dir.resolve("server.xml"), "old"); + StoreFileMover mover = new StoreFileMover(dir.toString(), "server.xml", "UTF-8"); + try (PrintWriter writer = mover.getWriter()) { + writer.print("new"); + } + mover.move(); + Assert.assertEquals("new", Files.readString(dir.resolve("server.xml"))); + Assert.assertFalse(Files.exists(dir.resolve("server.xml.new"))); + Path backup = mover.getConfigSave().toPath(); + Assert.assertTrue(Files.exists(backup)); + Assert.assertEquals("old", Files.readString(backup)); + } finally { + deleteRecursively(dir); + } + } + + /** + * Verify that without an existing configuration file, the new file is installed directly without creating a + * backup. + * + * @throws Exception if the test experiences an unexpected error + */ + @Test + public void testMoveWithoutExistingConfiguration() throws Exception { + Path dir = Files.createTempDirectory("storeFileMover"); + try { + StoreFileMover mover = new StoreFileMover(dir.toString(), "server.xml", "UTF-8"); + try (PrintWriter writer = mover.getWriter()) { + writer.print("new"); + } + mover.move(); + Assert.assertEquals("new", Files.readString(dir.resolve("server.xml"))); + Assert.assertFalse(Files.exists(mover.getConfigSave().toPath())); + } finally { + deleteRecursively(dir); + } + } + + /** + * Verify that a failure to create the backup leaves the active configuration file in place and unchanged. The + * backup is copied rather than renamed away, so no failure during the swap can leave the configuration file + * missing. + * + * @throws Exception if the test experiences an unexpected error + */ + @Test + public void testBackupFailureLeavesConfigurationInPlace() throws Exception { + Path dir = Files.createTempDirectory("storeFileMover"); + try { + Files.writeString(dir.resolve("server.xml"), "old"); + StoreFileMover mover = new StoreFileMover(dir.toString(), "server.xml", "UTF-8"); + try (PrintWriter writer = mover.getWriter()) { + writer.print("new"); + } + // Block the backup location so that the backup copy fails + Files.createDirectory(mover.getConfigSave().toPath()); + try { + mover.move(); + Assert.fail("move() should have failed"); + } catch (IOException ioe) { + // Expected, and it must report the failed backup copy + Assert.assertTrue(ioe.getMessage(), ioe.getMessage().contains("Failed to copy")); + } + Assert.assertEquals("old", Files.readString(dir.resolve("server.xml"))); + Assert.assertTrue(Files.exists(dir.resolve("server.xml.new"))); + } finally { + deleteRecursively(dir); + } + } + + private static void deleteRecursively(Path dir) throws IOException { + try (Stream<Path> paths = Files.walk(dir)) { + for (Path path : (Iterable<Path>) paths.sorted(Comparator.reverseOrder())::iterator) { + Files.deleteIfExists(path); + } + } + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
