SOLR-8308: Core gets inaccessible after RENAME operation with special characters

solr/CHANGES.txt

@@ -144,6 +144,9 @@ New Features
 
 * SOLR-8285: Ensure the /export handler works with NULL field values (Joel Bernstein)
 
+* SOLR-8308: Core gets inaccessible after RENAME operation with special characters
+  (Erik Hatcher, Erick Erickson)
+  
 Bug Fixes
 ----------------------
 * SOLR-8386: Add field option in the new admin UI schema page loads up even when no schemaFactory has been
@@ -590,6 +593,8 @@ Other Changes
 * SOLR-8600: add & use ReRankQParserPlugin parameter [default] constants,
   changed ReRankQuery.toString to use StringBuilder. (Christine Poerschke)
 
+* SOLR-8308: Core gets inaccessible after RENAME operation with special characters.
+
 ==================  5.4.1 ==================
 
 Bug Fixes

solr/core/src/java/org/apache/solr/core/CoreContainer.java

@@ -32,6 +32,7 @@ import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Future;
+import java.util.regex.Pattern;
 
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Maps;
@@ -113,12 +114,12 @@ public class CoreContainer {
   protected Properties containerProperties;
 
   private ConfigSetService coreConfigService;
-  
+
   protected ZkContainer zkSys = new ZkContainer();
   protected ShardHandlerFactory shardHandlerFactory;
-  
+
   private UpdateShardHandler updateShardHandler;
-  
+
   private ExecutorService coreContainerWorkExecutor = ExecutorUtil.newMDCAwareCachedThreadPool(
       new DefaultSolrThreadFactory("coreContainerWorkExecutor") );
 
@@ -131,7 +132,7 @@ public class CoreContainer {
   protected final String solrHome;
 
   protected final CoresLocator coresLocator;
-  
+
   private String hostName;
 
   private final JarRepository jarRepository = new JarRepository(this);
@@ -207,7 +208,7 @@ public class CoreContainer {
   public CoreContainer(NodeConfig config, Properties properties) {
     this(config, properties, new CorePropertiesLocator(config.getCoreRootDirectory()));
   }
-  
+
   public CoreContainer(NodeConfig config, Properties properties, boolean asyncSolrCoreLoad) {
     this(config, properties, new CorePropertiesLocator(config.getCoreRootDirectory()), asyncSolrCoreLoad);
   }
@@ -215,7 +216,7 @@ public class CoreContainer {
   public CoreContainer(NodeConfig config, Properties properties, CoresLocator locator) {
     this(config, properties, locator, false);
   }
-  
+
   public CoreContainer(NodeConfig config, Properties properties, CoresLocator locator, boolean asyncSolrCoreLoad) {
     this.loader = config.getSolrResourceLoader();
     this.solrHome = loader.getInstancePath().toString();
@@ -335,7 +336,7 @@ public class CoreContainer {
   /**
    * This method allows subclasses to construct a CoreContainer
    * without any default init behavior.
-   * 
+   *
    * @param testConstructor pass (Object)null.
    * @lucene.experimental
    */
@@ -350,7 +351,7 @@ public class CoreContainer {
   public static CoreContainer createAndLoad(Path solrHome) {
     return createAndLoad(solrHome, solrHome.resolve(SolrXmlConfig.SOLR_XML_FILE));
   }
-  
+
   /**
    * Create a new CoreContainer and load its cores
    * @param solrHome the solr home directory
@@ -368,7 +369,7 @@ public class CoreContainer {
     }
     return cc;
   }
-  
+
   public Properties getContainerProperties() {
     return containerProperties;
   }
@@ -463,7 +464,7 @@ public class CoreContainer {
                 if (zkSys.getZkController() != null) {
                   zkSys.getZkController().throwErrorIfReplicaReplaced(cd);
                 }
-                
+
                 core = create(cd, false);
               } finally {
                 if (asyncSolrCoreLoad) {
@@ -510,7 +511,7 @@ public class CoreContainer {
         ExecutorUtil.shutdownAndAwaitTermination(coreLoadExecutor);
       }
     }
-    
+
     if (isZooKeeperAware()) {
       zkSys.getZkController().checkOverseerDesignate();
     }
@@ -536,7 +537,7 @@ public class CoreContainer {
   }
 
   private volatile boolean isShutDown = false;
-  
+
   public boolean isShutDown() {
     return isShutDown;
   }
@@ -547,11 +548,11 @@ public class CoreContainer {
   public void shutdown() {
     log.info("Shutting down CoreContainer instance="
         + System.identityHashCode(this));
-    
+
     isShutDown = true;
-    
+
     ExecutorUtil.shutdownAndAwaitTermination(coreContainerWorkExecutor);
-    
+
     if (isZooKeeperAware()) {
       cancelCoreRecoveries();
       zkSys.publishCoresAsDown(solrCores.getCores());
@@ -640,7 +641,7 @@ public class CoreContainer {
       }
     }
   }
-  
+
   @Override
   protected void finalize() throws Throwable {
     try {
@@ -655,17 +656,26 @@ public class CoreContainer {
   public CoresLocator getCoresLocator() {
     return coresLocator;
   }
-  
+
+  // Insure that the core name won't cause problems later on.
+  final static Pattern corePattern = Pattern.compile("^[\\._A-Za-z0-9]*$");
+
+
+  public void validateCoreName(String name) {
+    if (name == null || !corePattern.matcher(name).matches()) {
+      throw new IllegalArgumentException("Invalid core name: '" + name +
+          "' Names must consist entirely of periods, underscores and alphanumerics");
+    }
+  }
+
+
+
   protected SolrCore registerCore(String name, SolrCore core, boolean registerInZk) {
     if( core == null ) {
       throw new RuntimeException( "Can not register a null core." );
     }
-    if( name == null ||
-        name.indexOf( '/'  ) >= 0 ||
-        name.indexOf( '\\' ) >= 0 ){
-      throw new RuntimeException( "Invalid core name: "+name );
-    }
-    // We can register a core when creating them via the admin UI, so we need to insure that the dynamic descriptors
+
+    // We can register a core when creating them via the admin UI, so we need to ensure that the dynamic descriptors
     // are up to date
     CoreDescriptor cd = core.getCoreDescriptor();
     if ((cd.isTransient() || ! cd.isLoadOnStartup())
@@ -808,6 +818,7 @@ public class CoreContainer {
     SolrCore core = null;
     try {
       MDCLoggingContext.setCore(core);
+      validateCoreName(dcore.getName());
       if (zkSys.getZkController() != null) {
         zkSys.getZkController().preRegister(dcore);
       }
@@ -1010,6 +1021,7 @@ public class CoreContainer {
   }
 
   public void rename(String name, String toName) {
+    validateCoreName(toName);
     try (SolrCore core = getCore(name)) {
       if (core != null) {
         registerCore(toName, core, true);

solr/core/src/test/org/apache/solr/handler/admin/CoreAdminHandlerTest.java

@@ -128,10 +128,25 @@ public class CoreAdminHandlerTest extends SolrTestCaseJ4 {
     assertTrue("instDir doesn't exist: " + instDir, Files.exists(instDir));
     final File instPropFile = new File(workDir, "instProp");
     FileUtils.copyDirectory(instDir.toFile(), instPropFile);
-    
-    // create a new core (using CoreAdminHandler) w/ properties
-    
+
     SolrQueryResponse resp = new SolrQueryResponse();
+    // Sneaking in a test for using a bad core name
+    try {
+      admin.handleRequestBody
+          (req(CoreAdminParams.ACTION,
+              CoreAdminParams.CoreAdminAction.CREATE.toString(),
+              CoreAdminParams.INSTANCE_DIR, instPropFile.getAbsolutePath(),
+              CoreAdminParams.NAME, "ugly$core=name"),
+              resp);
+
+    } catch (SolrException se) {
+      assertTrue("Expected error message for bad core name.", se.toString().contains("Invalid core name"));
+    }
+    CoreDescriptor cd = cores.getCoreDescriptor("ugly$core=name");
+    assertNull("Should NOT have added this core!", cd);
+
+    // create a new core (using CoreAdminHandler) w/ properties
+
     admin.handleRequestBody
       (req(CoreAdminParams.ACTION,
            CoreAdminParams.CoreAdminAction.CREATE.toString(),
@@ -142,7 +157,7 @@ public class CoreAdminHandlerTest extends SolrTestCaseJ4 {
        resp);
     assertNull("Exception on create", resp.getException());
 
-    CoreDescriptor cd = cores.getCoreDescriptor("props");
+    cd = cores.getCoreDescriptor("props");
     assertNotNull("Core not added!", cd);
     assertEquals(cd.getCoreProperty("hoss", null), "man");
     assertEquals(cd.getCoreProperty("foo", null), "baz");
@@ -188,7 +203,43 @@ public class CoreAdminHandlerTest extends SolrTestCaseJ4 {
     assertEquals("bogus_dir_core status isn't empty",
                  0, ((NamedList)status.get("bogus_dir_core")).size());
 
-               
+
+    //Try renaming the core, we should fail
+    // First assert that the props core exists
+    cd = cores.getCoreDescriptor("props");
+    assertNotNull("Core disappeared!", cd);
+
+    // now rename it something else just for kicks since we don't actually test this that I could find.
+    admin.handleRequestBody
+        (req(CoreAdminParams.ACTION,
+            CoreAdminParams.CoreAdminAction.RENAME.toString(),
+            CoreAdminParams.CORE, "props",
+            CoreAdminParams.OTHER, "rename_me"),
+            resp);
+
+    cd = cores.getCoreDescriptor("rename_me");
+    assertNotNull("Core should have been renamed!", cd);
+
+    // Rename it something bogus and see if you get an exception, the old core is still there and the bogus one isn't
+    try {
+      admin.handleRequestBody
+          (req(CoreAdminParams.ACTION,
+              CoreAdminParams.CoreAdminAction.RENAME.toString(),
+              CoreAdminParams.CORE, "rename_me",
+              CoreAdminParams.OTHER, "bad$name"),
+              resp);
+    } catch (IllegalArgumentException iae) { // why the heck does create return a SolrException (admittedly wrapping an IAE)
+      assertTrue("Expected error message for bad core name.", iae.getMessage().contains("Invalid core name"));
+    }
+
+    cd = cores.getCoreDescriptor("bad$name");
+    assertNull("Core should NOT exist!", cd);
+
+    cd = cores.getCoreDescriptor("rename_me");
+    assertNotNull("Core should have been renamed!", cd);
+
+
+
     // :TODO: because of SOLR-3665 we can't ask for status from all cores
 
   }