Sanity check the version specified in the mod file (#8749)

* Sanity check the version specified in the mod file to
make sure it's compatible with JPMS standards for
version strings.

Closes #8748

Requires SPI 6

build.gradle

@@ -39,7 +39,7 @@ ext {
     MAPPING_VERSION = '1.19'
     MC_VERSION = '1.19'
     MCP_VERSION = '20220607.102129'
-    SPI_VERSION = '5.0.3'
+    SPI_VERSION = '6.0.0'
     ACCESSTRANSFORMERS_VERSION = '8.0.4'
     COREMODS_VERSION = '5.0.1'
     EVENTBUS_VERSION = '6.0.0'

fmlloader/src/main/java/net/minecraftforge/fml/loading/LoadingModList.java

@@ -10,11 +10,18 @@ import net.minecraftforge.fml.loading.moddiscovery.BackgroundScanHandler;
 import net.minecraftforge.fml.loading.moddiscovery.ModFile;
 import net.minecraftforge.fml.loading.moddiscovery.ModFileInfo;
 import net.minecraftforge.fml.loading.moddiscovery.ModInfo;
+import net.minecraftforge.forgespi.locating.IModFile;
 
 import java.net.URL;
 import java.nio.file.Files;
 import java.nio.file.Path;
-import java.util.*;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Enumeration;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.NoSuchElementException;
 import java.util.stream.Collectors;
 
 /**
@@ -28,7 +35,7 @@ public class LoadingModList
     private final List<ModInfo> sortedList;
     private final Map<String, ModFileInfo> fileById;
     private final List<EarlyLoadingException> preLoadErrors;
-    private List<ModFile> brokenFiles;
+    private List<IModFile> brokenFiles;
 
     private LoadingModList(final List<ModFile> modFiles, final List<ModInfo> sortedList)
     {
@@ -154,11 +161,11 @@ public class LoadingModList
         return preLoadErrors;
     }
 
-    public void setBrokenFiles(final List<ModFile> brokenFiles) {
+    public void setBrokenFiles(final List<IModFile> brokenFiles) {
         this.brokenFiles = brokenFiles;
     }
 
-    public List<ModFile> getBrokenFiles() {
+    public List<IModFile> getBrokenFiles() {
         return this.brokenFiles;
     }
 }

fmlloader/src/main/java/net/minecraftforge/fml/loading/moddiscovery/AbstractJarFileDependencyLocator.java

@@ -8,6 +8,7 @@ package net.minecraftforge.fml.loading.moddiscovery;
 import com.mojang.logging.LogUtils;
 import net.minecraftforge.forgespi.locating.IDependencyLocator;
 import net.minecraftforge.forgespi.locating.IModFile;
+import net.minecraftforge.forgespi.locating.ModFileLoadingException;
 import org.slf4j.Logger;
 
 import java.io.FileNotFoundException;
@@ -37,11 +38,11 @@ public abstract class AbstractJarFileDependencyLocator extends AbstractJarFileMo
     protected Optional<IModFile> loadModFileFrom(final IModFile file, final Path path) {
         try {
             final Path pathInModFile = file.findResource(path.toString());
-            return createMod(pathInModFile);
+            return Optional.of(createMod(pathInModFile).file());
         }
         catch (Exception e) {
             LOGGER.error("Failed to load mod file {} from {}", path, file.getFileName());
-            return Optional.empty();
+            throw new ModFileLoadingException("Failed to load mod file "+file.getFileName());
         }
     }
 

fmlloader/src/main/java/net/minecraftforge/fml/loading/moddiscovery/AbstractJarFileModLocator.java

@@ -5,25 +5,18 @@
 
 package net.minecraftforge.fml.loading.moddiscovery;
 
-import net.minecraftforge.forgespi.locating.IModFile;
 import net.minecraftforge.forgespi.locating.IModLocator;
 
 import java.nio.file.Path;
 import java.util.List;
-import java.util.Optional;
-import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
 public abstract class AbstractJarFileModLocator extends AbstractJarFileModProvider implements IModLocator
 {
     @Override
-    public List<IModFile> scanMods()
+    public List<IModLocator.ModFileOrException> scanMods()
     {
-        return scanCandidates()
-                .map(this::createMod)
-                .filter(Optional::isPresent)
-                .map(Optional::get)
-                .collect(Collectors.toList());
+        return scanCandidates().map(this::createMod).toList();
     }
 
     public abstract Stream<Path> scanCandidates();

fmlloader/src/main/java/net/minecraftforge/fml/loading/moddiscovery/AbstractModProvider.java

@@ -13,10 +13,13 @@ import net.minecraftforge.forgespi.language.IConfigurable;
 import net.minecraftforge.forgespi.language.IModFileInfo;
 import net.minecraftforge.forgespi.language.IModInfo;
 import net.minecraftforge.forgespi.locating.IModFile;
+import net.minecraftforge.forgespi.locating.IModLocator;
 import net.minecraftforge.forgespi.locating.IModProvider;
+import net.minecraftforge.forgespi.locating.ModFileLoadingException;
 import org.slf4j.Logger;
 
 import java.nio.file.Path;
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
@@ -30,7 +33,7 @@ public abstract class AbstractModProvider implements IModProvider
     protected static final String MODS_TOML = "META-INF/mods.toml";
     protected static final String MANIFEST = "META-INF/MANIFEST.MF";
 
-    protected Optional<IModFile> createMod(Path... path) {
+    protected IModLocator.ModFileOrException createMod(Path... path) {
         var mjm = new ModJarMetadata();
         var sj = SecureJar.from(
                 Manifest::new,
@@ -44,18 +47,22 @@ public abstract class AbstractModProvider implements IModProvider
         if (type == null) {
             type = getDefaultJarModType();
         }
-        if (sj.findFile(MODS_TOML).isPresent()) {
-            LOGGER.debug(LogMarkers.SCAN, "Found {} mod of type {}: {}", MODS_TOML, type, path);
-            mod = new ModFile(sj, this, ModFileParser::modsTomlParser);
-        } else if (type != null) {
-            LOGGER.debug(LogMarkers.SCAN, "Found {} mod of type {}: {}", MANIFEST, type, path);
-            mod = new ModFile(sj, this, this::manifestParser, type);
-        } else {
-            return Optional.empty();
+        try {
+            if (sj.findFile(MODS_TOML).isPresent()) {
+                LOGGER.debug(LogMarkers.SCAN, "Found {} mod of type {}: {}", MODS_TOML, type, path);
+                mod = new ModFile(sj, this, ModFileParser::modsTomlParser);
+            } else if (type != null) {
+                LOGGER.debug(LogMarkers.SCAN, "Found {} mod of type {}: {}", MANIFEST, type, path);
+                mod = new ModFile(sj, this, this::manifestParser, type);
+            } else {
+                return new IModLocator.ModFileOrException(null, new ModFileLoadingException("Invalid mod file found "+ Arrays.toString(path)));
+            }
+        } catch (InvalidModFileException e) {
+            return new IModLocator.ModFileOrException(null, e);
         }
 
         mjm.setModFile(mod);
-        return Optional.of(mod);
+        return new IModLocator.ModFileOrException(mod, null);
     }
 
     protected IModFileInfo manifestParser(final IModFile mod) {

fmlloader/src/main/java/net/minecraftforge/fml/loading/moddiscovery/ExplodedDirectoryLocator.java

@@ -30,14 +30,14 @@ public class ExplodedDirectoryLocator implements IModLocator {
     private final Map<ExplodedMod, IModFile> mods = new HashMap<>();
 
     @Override
-    public List<IModFile> scanMods() {
+    public List<IModLocator.ModFileOrException> scanMods() {
         explodedMods.forEach(explodedMod ->
                 ModJarMetadata.buildFile(this,
                         jar->jar.findFile("/META-INF/mods.toml").isPresent(),
                         (a,b) -> true,
                         explodedMod.paths().toArray(Path[]::new))
                 .ifPresentOrElse(f->mods.put(explodedMod, f), () -> LOGGER.warn(LogMarkers.LOADING, "Failed to find exploded resource mods.toml in directory {}", explodedMod.paths().get(0).toString())));
-        return List.copyOf(mods.values());
+        return mods.values().stream().map(mf->new IModLocator.ModFileOrException(mf, null)).toList();
     }
 
     @Override

fmlloader/src/main/java/net/minecraftforge/fml/loading/moddiscovery/InvalidModFileException.java

@@ -6,16 +6,22 @@
 package net.minecraftforge.fml.loading.moddiscovery;
 
 import net.minecraftforge.forgespi.language.IModFileInfo;
+import net.minecraftforge.forgespi.locating.ModFileLoadingException;
 
 import java.util.Locale;
+import java.util.Optional;
 
-public class InvalidModFileException extends RuntimeException
+public class InvalidModFileException extends ModFileLoadingException
 {
     private final IModFileInfo modFileInfo;
 
     public InvalidModFileException(String message, IModFileInfo modFileInfo)
     {
-        super(String.format(Locale.ENGLISH, "%s (%s)", message, ((ModFileInfo)modFileInfo).getFile().getFileName()));
+        super(String.format(Locale.ROOT, "%s (%s)", message, Optional.ofNullable(modFileInfo).map(mf->mf.getFile().getFileName()).orElse("MISSING FILE NAME")));
         this.modFileInfo = modFileInfo;
     }
+
+    public IModFileInfo getBrokenFile() {
+        return modFileInfo;
+    }
 }

fmlloader/src/main/java/net/minecraftforge/fml/loading/moddiscovery/MinecraftLocator.java

@@ -17,8 +17,11 @@ import net.minecraftforge.forgespi.locating.ModFileFactory;
 import org.slf4j.Logger;
 
 import java.io.IOException;
-import java.nio.file.*;
-import java.util.*;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
 import java.util.function.Consumer;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
@@ -28,7 +31,7 @@ public class MinecraftLocator extends AbstractModProvider implements IModLocator
     private static final Logger LOGGER = LogUtils.getLogger();
 
     @Override
-    public List<IModFile> scanMods() {
+    public List<IModLocator.ModFileOrException> scanMods() {
         final var launchHandler = FMLLoader.getLaunchHandler();
         var baseMC = launchHandler.getMinecraftPaths();
         var mcjar = ModJarMetadata.buildFile(j->ModFileFactory.FACTORY.build(j, this, this::buildMinecraftTOML), j->true, baseMC.minecraftFilter(), baseMC.minecraftPaths().toArray(Path[]::new)).orElseThrow();
@@ -37,12 +40,11 @@ public class MinecraftLocator extends AbstractModProvider implements IModLocator
                 .map(sj -> new ModFile(sj, this, ModFileParser::modsTomlParser))
                 .collect(Collectors.<IModFile>toList());
         var othermods = baseMC.otherModPaths().stream()
-                .map(p -> createMod(p.toArray(Path[]::new)).orElse(null))
-                .filter(Objects::nonNull)
-                .toList();
+                .map(p -> createMod(p.toArray(Path[]::new)))
+                .filter(Objects::nonNull);
         artifacts.add(mcjar);
-        artifacts.addAll(othermods);
-        return artifacts;
+
+        return Stream.concat(artifacts.stream().map(f -> new ModFileOrException(f, null)), othermods).toList();
     }
 
     private IModFileInfo buildMinecraftTOML(final IModFile iModFile) {

fmlloader/src/main/java/net/minecraftforge/fml/loading/moddiscovery/ModDiscoverer.java

@@ -16,6 +16,7 @@ import net.minecraftforge.fml.loading.LogMarkers;
 import net.minecraftforge.fml.loading.UniqueModListBuilder;
 import net.minecraftforge.fml.loading.progress.StartupMessageManager;
 import net.minecraftforge.forgespi.Environment;
+import net.minecraftforge.forgespi.language.IModFileInfo;
 import net.minecraftforge.forgespi.locating.IDependencyLocator;
 import net.minecraftforge.forgespi.locating.IModFile;
 import net.minecraftforge.forgespi.locating.IModLocator;
@@ -24,6 +25,7 @@ import org.slf4j.Logger;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import java.util.ServiceLoader;
 import java.util.stream.Collectors;
 
@@ -63,18 +65,33 @@ public class ModDiscoverer {
         List<ModFile> loadedFiles = new ArrayList<>();
         List<EarlyLoadingException.ExceptionData> discoveryErrorData = new ArrayList<>();
         boolean successfullyLoadedMods = true;
+        List<IModFileInfo> brokenFiles = new ArrayList<>();
 
         //Loop all mod locators to get the prime mods to load from.
         for (IModLocator locator : modLocatorList) {
             try {
-                LOGGER.debug(LogMarkers.SCAN,"Trying locator {}", locator);
-                var locatedFiles = locator.scanMods();
-
-                if (locatedFiles.stream().anyMatch(file -> !(file instanceof ModFile))) {
-                    LOGGER.error(LogMarkers.SCAN, "A mod locator returned a file which is not a ModFile instance!. They will be skipped!");
+                LOGGER.debug(LogMarkers.SCAN, "Trying locator {}", locator);
+                var candidates = locator.scanMods();
+                LOGGER.debug(LogMarkers.SCAN, "Locator {} found {} candidates or errors", locator, candidates.size());
+                var exceptions = candidates.stream().map(IModLocator.ModFileOrException::ex).filter(Objects::nonNull).toList();
+                if (!exceptions.isEmpty()) {
+                    LOGGER.debug(LogMarkers.SCAN, "Locator {} found {} invalid mod files", locator, exceptions.size());
+                    brokenFiles.addAll(exceptions.stream().map(e->e instanceof InvalidModFileException ime ? ime.getBrokenFile() : null).filter(Objects::nonNull).toList());
                 }
+                var locatedFiles = candidates.stream().map(IModLocator.ModFileOrException::file).filter(Objects::nonNull).toList();
 
+                var badModFiles = locatedFiles.stream().filter(file -> !(file instanceof ModFile)).toList();
+                if (!badModFiles.isEmpty()) {
+                    LOGGER.error(LogMarkers.SCAN, "Locator {} returned {} files which is are not ModFile instances! They will be skipped!", locator, badModFiles.size());
+                    brokenFiles.addAll(badModFiles.stream().map(mf->mf.getModFileInfo()).toList());
+                }
+                locatedFiles.removeAll(badModFiles);
+                LOGGER.debug(LogMarkers.SCAN, "Locator {} found {} valid mod files", locator, locatedFiles.size());
                 handleLocatedFiles(loadedFiles, locatedFiles);
+            } catch (InvalidModFileException imfe) {
+                // We don't generally expect this exception, since it should come from the candidates stream above and be handled in the Locator, but just in case.
+                LOGGER.error(LogMarkers.SCAN, "Locator {} found an invalid mod file {}", locator, imfe.getBrokenFile(), imfe);
+                brokenFiles.add(imfe.getBrokenFile());
             } catch (EarlyLoadingException exception) {
                 LOGGER.error(LogMarkers.SCAN, "Failed to load mods with locator {}", locator, exception);
                 discoveryErrorData.addAll(exception.getAllData());
@@ -141,7 +158,7 @@ public class ModDiscoverer {
 
         //Validate the loading. With a deduplicated list, we can now successfully process the artifacts and load
         //transformer plugins.
-        var validator = new ModValidator(modFilesMap, discoveryErrorData);
+        var validator = new ModValidator(modFilesMap, brokenFiles, discoveryErrorData);
         validator.stage1Validation();
         return validator;
     }

fmlloader/src/main/java/net/minecraftforge/fml/loading/moddiscovery/ModInfo.java

@@ -31,6 +31,7 @@ public class ModInfo implements IModInfo, IConfigurable
     private static final DefaultArtifactVersion DEFAULT_VERSION = new DefaultArtifactVersion("1");
     private static final Pattern VALID_MODID = Pattern.compile("^[a-z][a-z0-9_]{1,63}$");
     private static final Pattern VALID_NAMESPACE = Pattern.compile("^[a-z][a-z0-9_.-]{1,63}$");
+    private static final Pattern VALID_VERSION = Pattern.compile("^\\d+.*");
 
     private final ModFileInfo owningFile;
     private final String modId;
@@ -97,6 +98,11 @@ public class ModInfo implements IModInfo, IConfigurable
 
         this.modUrl = config.<String>getConfigElement("modUrl")
                 .map(StringUtils::toURL);
+
+        // verify we have a valid mod version otherwise throw an exception
+        if (!VALID_VERSION.matcher(this.version.toString()).matches()) {
+            throw new InvalidModFileException("Illegal version number specified "+this.version, this.getOwningFile());
+        }
     }
 
     @Override

fmlloader/src/main/java/net/minecraftforge/fml/loading/moddiscovery/ModValidator.java

@@ -13,11 +13,16 @@ import net.minecraftforge.fml.loading.LoadingModList;
 import net.minecraftforge.fml.loading.LogMarkers;
 import net.minecraftforge.fml.loading.ModSorter;
 import net.minecraftforge.fml.loading.progress.StartupMessageManager;
+import net.minecraftforge.forgespi.language.IModFileInfo;
 import net.minecraftforge.forgespi.locating.IModFile;
 import org.jetbrains.annotations.NotNull;
 import org.slf4j.Logger;
 
-import java.util.*;
+import java.util.ArrayList;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.stream.Collectors;
 
 public class ModValidator {
     private static final Logger LOGGER = LogUtils.getLogger();
@@ -25,16 +30,17 @@ public class ModValidator {
     private final List<ModFile> candidatePlugins;
     private final List<ModFile> candidateMods;
     private LoadingModList loadingModList;
-    private List<ModFile> brokenFiles;
+    private List<IModFile> brokenFiles;
     private final List<EarlyLoadingException.ExceptionData> discoveryErrorData;
 
-    public ModValidator(final Map<IModFile.Type, List<ModFile>> modFiles, final List<EarlyLoadingException.ExceptionData> discoveryErrorData) {
+    public ModValidator(final Map<IModFile.Type, List<ModFile>> modFiles, final List<IModFileInfo> brokenFiles, final List<EarlyLoadingException.ExceptionData> discoveryErrorData) {
         this.modFiles = modFiles;
         this.candidateMods = lst(modFiles.get(IModFile.Type.MOD));
         this.candidateMods.addAll(lst(modFiles.get(IModFile.Type.GAMELIBRARY)));
         this.candidatePlugins = lst(modFiles.get(IModFile.Type.LANGPROVIDER));
         this.candidatePlugins.addAll(lst(modFiles.get(IModFile.Type.LIBRARY)));
         this.discoveryErrorData = discoveryErrorData;
+        this.brokenFiles = brokenFiles.stream().map(IModFileInfo::getFile).collect(Collectors.toList()); // mutable list
     }
 
     private static List<ModFile> lst(List<ModFile> files) {
@@ -42,7 +48,7 @@ public class ModValidator {
     }
 
     public void stage1Validation() {
-        brokenFiles = validateFiles(candidateMods);
+        brokenFiles.addAll(validateFiles(candidateMods));
         if (LOGGER.isDebugEnabled(LogMarkers.SCAN)) {
             LOGGER.debug(LogMarkers.SCAN, "Found {} mod files with {} mods", candidateMods.size(), candidateMods.stream().mapToInt(mf -> mf.getModInfos().size()).sum());
         }