FELIX-6515 Fix race condition when rewriting config files in ConfigInstaller

fileinstall/src/main/java/org/apache/felix/fileinstall/internal/ConfigInstaller.java

@@ -19,22 +19,26 @@
 package org.apache.felix.fileinstall.internal;
 
 import java.io.BufferedInputStream;
+import java.io.ByteArrayOutputStream;
 import java.io.File;
 import java.io.FileInputStream;
 import java.io.FileOutputStream;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.InputStreamReader;
+import java.io.OutputStream;
 import java.io.OutputStreamWriter;
 import java.io.Reader;
 import java.io.Writer;
 import java.lang.reflect.Array;
 import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
 import java.net.URI;
+import java.nio.file.Files;
 import java.security.AccessController;
 import java.security.PrivilegedAction;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Dictionary;
 import java.util.Enumeration;
 import java.util.HashMap;
@@ -288,13 +292,35 @@ public void doConfigurationEvent(ConfigurationEvent configurationEvent)
                     {
                         props.remove(key);
                     }
-                    try (Writer fw = new OutputStreamWriter(new FileOutputStream(file), encoding()))
+
+                    ByteArrayOutputStream baos = new ByteArrayOutputStream();
+                    try (Writer fw = new OutputStreamWriter(baos, encoding()))
                     {
                         props.save( fw );
                     }
-                    // we're just writing out what's already loaded into ConfigAdmin, so
-                    // update file checksum since lastModified gets updated when writing
-                    fileInstall.updateChecksum(file);
+                    byte[] newContent = baos.toByteArray();
+
+                    // re-writing a config file must be guarded by a write lock as otherwise DirectoryWatcher
+                    // might pick up an empty file and as a result, write this file to disk in a follow-up event
+                    fileInstall.lock.writeLock().lockInterruptibly();
+                    try
+                    {
+                        // do not touch the file (and its lastModified) if there is nothing to change
+                        if (!Arrays.equals(newContent, Files.readAllBytes(file.toPath())))
+                        {
+                            try (OutputStream os = new FileOutputStream(file))
+                            {
+                                os.write(newContent);
+                            }
+                            // we're just writing out what's already loaded into ConfigAdmin, so
+                            // update file checksum since lastModified gets updated when writing
+                            fileInstall.updateChecksum(file);
+                        }
+                    }
+                    finally
+                    {
+                        fileInstall.lock.writeLock().unlock();
+                    }
                 }
             }
             catch (Exception e)

fileinstall/src/test/java/org/apache/felix/fileinstall/internal/ConfigInstallerTest.java

@@ -27,6 +27,7 @@
 import java.util.Dictionary;
 import java.util.Hashtable;
 import java.util.concurrent.atomic.AtomicReference;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
 
 import junit.framework.TestCase;
 import org.easymock.Capture;
@@ -435,6 +436,108 @@ public Boolean answer() throws Throwable {
         assertEquals("networkInterface = \"wlp3s1\"", Files.readAllLines( Paths.get(file.toURI()) ).get(0));
     }
 
+    /**
+     * FELIX-6515: rewriting a config file truncates it before filling it. The write must be guarded by the
+     * global write lock so that a DirectoryWatcher (which holds the read lock while processing) can never
+     * observe, and subsequently write back, an empty file.
+     */
+    public void testDoConfigurationEventRewritesFileUnderWriteLock() throws Exception
+    {
+        final File file = File.createTempFile("test", ".config");
+        try (OutputStream os = new FileOutputStream(file)) {
+            os.write("networkInterface = \"wlp3s1\"\n".getBytes("UTF-8"));
+        }
+
+        final String pid = file.getName().substring(0, file.getName().indexOf(".config"));
+
+        final ServiceReference<ConfigurationAdmin> sr = EasyMock.createMock(ServiceReference.class);
+
+        final AtomicReference<ReentrantReadWriteLock> lockRef = new AtomicReference<>();
+
+        EasyMock.expect(mockBundleContext.getBundle()).andReturn(mockBundle).anyTimes();
+        EasyMock.expect(mockBundle.loadClass(ConfigurationAttribute.class.getName())).andReturn((Class)ConfigurationAttribute.class).anyTimes();
+        EasyMock.expect(mockBundleContext.getProperty((String) EasyMock.anyObject())).andReturn(null).anyTimes();
+
+        final Dictionary<String, Object> configProps = new Hashtable<String, Object>() {
+            {
+                put("networkInterface", "wlp3s0");
+                put(DirectoryWatcher.FILENAME, file.toURI().toString());
+            }
+        };
+        EasyMock.expect(mockConfiguration.getProperties()).andReturn(configProps).anyTimes();
+        EasyMock.expect(mockConfiguration.getPid()).andReturn(pid).anyTimes();
+        EasyMock.expect(mockConfigurationAdmin.listConfigurations("(service.pid=" + pid + ")"))
+                .andReturn(new Configuration[] { mockConfiguration });
+
+        EasyMock.replay(mockBundleContext, mockConfigurationAdmin, mockBundle, mockConfiguration, sr);
+
+        final AtomicReference<Boolean> lockedOnChecksumUpdate = new AtomicReference<>();
+        final FileInstall fileInstall = new FileInstall() {
+            @Override
+            public void updateChecksum(File f) {
+                lockedOnChecksumUpdate.set(lockRef.get().isWriteLockedByCurrentThread());
+            }
+        };
+        final ReentrantReadWriteLock lock = (ReentrantReadWriteLock) fileInstall.lock;
+        lockRef.set(lock);
+        final ConfigInstaller ci = new ConfigInstaller( mockBundleContext, mockConfigurationAdmin, fileInstall );
+
+        ci.doConfigurationEvent( new ConfigurationEvent(sr , ConfigurationEvent.CM_UPDATED, null, pid ) );
+
+        assertEquals("networkInterface = \"wlp3s0\"", Files.readAllLines( Paths.get(file.toURI()) ).get(0));
+        assertEquals("Checksum must be updated before the write lock is released", Boolean.TRUE, lockedOnChecksumUpdate.get());
+        assertFalse("Write lock must be released after write-back", lock.isWriteLocked());
+    }
+
+    /**
+     * FELIX-6515: a config file whose content would not change must not be touched at all.
+     */
+    public void testDoConfigurationEventDoesNotRewriteUnchangedFile() throws Exception
+    {
+        final File file = File.createTempFile("test", ".config");
+        try (OutputStream os = new FileOutputStream(file)) {
+            os.write("networkInterface = \"wlp3s0\"\n".getBytes("UTF-8"));
+        }
+        final long lastModified = 1_000_000_000L;
+        assertTrue(file.setLastModified(lastModified));
+
+        final String pid = file.getName().substring(0, file.getName().indexOf(".config"));
+        final ServiceReference<ConfigurationAdmin> sr = EasyMock.createMock(ServiceReference.class);
+
+        EasyMock.expect(mockBundleContext.getBundle()).andReturn(mockBundle).anyTimes();
+        EasyMock.expect(mockBundle.loadClass(ConfigurationAttribute.class.getName())).andReturn((Class)ConfigurationAttribute.class).anyTimes();
+        EasyMock.expect(mockBundleContext.getProperty((String) EasyMock.anyObject())).andReturn(null).anyTimes();
+
+        final Dictionary<String, Object> configProps = new Hashtable<String, Object>() {
+            {
+                put("networkInterface", "wlp3s0");
+                put(DirectoryWatcher.FILENAME, file.toURI().toString());
+            }
+        };
+        EasyMock.expect(mockConfiguration.getProperties()).andReturn(configProps).anyTimes();
+        EasyMock.expect(mockConfiguration.getPid()).andReturn(pid).anyTimes();
+        EasyMock.expect(mockConfigurationAdmin.listConfigurations("(service.pid=" + pid + ")"))
+                .andReturn(new Configuration[] { mockConfiguration });
+
+        EasyMock.replay(mockBundleContext, mockConfigurationAdmin, mockBundle, mockConfiguration, sr);
+
+        final AtomicReference<Boolean> checksumUpdated = new AtomicReference<>(false);
+        final FileInstall fileInstall = new FileInstall() {
+            @Override
+            public void updateChecksum(File f) {
+                checksumUpdated.set(true);
+            }
+        };
+        final ConfigInstaller ci = new ConfigInstaller( mockBundleContext, mockConfigurationAdmin, fileInstall );
+
+        ci.doConfigurationEvent( new ConfigurationEvent(sr , ConfigurationEvent.CM_UPDATED, null, pid ) );
+
+        assertEquals("Unchanged file must not be modified", lastModified, file.lastModified());
+        assertFalse("Checksum must not be updated for an unchanged file", checksumUpdated.get());
+    }
+
+
+
     public void testSetConfiguration() throws Exception
     {
         EasyMock.expect(mockBundleContext.getBundle()).andReturn(mockBundle).anyTimes();