ConsoleService: guard against infinite loops

The design of the ConsoleArgument handlers allows each handler to remove
one or more arguments from the beginning of the linked argument list,
which provides for a nice sequential processing sequence.

However, it is very easy to code a ConsoleArgument and then forget to
remove any elements from the list. Previously, such a bug would cause
the ConsoleService's processArgs method to never return, instead calling
the same handler over and over.

This change adds a rudimentary safeguard against such an occurrence: if
the list of arguments is totally unchanged after a ConsoleArgument
plugin has supposedly handled it, then we warn the user about the
improper handling, remove the first argument from the list, and proceed.
In this way, infinite loops should be a lot less likely; a bogus
ConsoleArgument plugin would now need to mutate the list (e.g., add an
argument rather than remove one) in order to cause trouble.

Author: ctrueden

src/main/java/org/scijava/console/DefaultConsoleService.java

@@ -35,6 +35,7 @@
 import java.io.PrintStream;
 import java.util.ArrayList;
 import java.util.LinkedList;
+import java.util.List;
 
 import org.scijava.Context;
 import org.scijava.console.OutputEvent.Source;
@@ -82,6 +83,8 @@ public void processArgs(final String... args) {
 			argList.add(arg);
 		}
 
+		final List<String> previousArgs = new ArrayList<>();
+
 		while (!argList.isEmpty()) {
 			final ConsoleArgument handler = getHandler(argList);
 			if (handler == null) {
@@ -90,7 +93,22 @@ public void processArgs(final String... args) {
 				log.warn("Ignoring invalid argument: " + arg);
 				continue;
 			}
+
+			// keep a copy of the argument list prior to handling
+			previousArgs.clear();
+			previousArgs.addAll(argList);
+
+			// process the argument
 			handler.handle(argList);
+
+			// verify that the handler did something to the list;
+			// this guards against bugs which would cause infinite loops
+			if (sameElements(previousArgs, argList)) {
+				// skip improperly handled argument
+				final String arg = argList.removeFirst();
+				log.warn("Plugin '" + handler.getClass().getName() +
+					"' failed to handle argument: " + arg);
+			}
 		}
 	}
 
@@ -174,6 +192,23 @@ private MultiPrintStream multiPrintStream(final PrintStream ps) {
 		return new MultiPrintStream(ps);
 	}
 
+	/**
+	 * Gets whether two lists have exactly the same elements in them.
+	 * <p>
+	 * We cannot use {@link List#equals(Object)} because want to check for
+	 * identical references, not per-element object equality.
+	 * </p>
+	 */
+	private boolean sameElements(final List<String> l1,
+		final List<String> l2)
+	{
+		if (l1.size() != l2.size()) return false;
+		for (int i = 0; i < l1.size(); i++) {
+			if (l1.get(i) != l2.get(i)) return false;
+		}
+		return true;
+	}
+
 	// -- Helper classes --
 
 	/**

src/test/java/org/scijava/console/ConsoleServiceTest.java

@@ -32,6 +32,7 @@
 package org.scijava.console;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
 
@@ -72,10 +73,23 @@ public void tearDown() {
 	/** Tests {@link ConsoleService#processArgs(String...)}. */
 	@Test
 	public void testProcessArgs() {
+		assertFalse(consoleService.getInstance(FooArgument.class).argsHandled);
 		consoleService.processArgs("--foo", "--bar");
 		assertTrue(consoleService.getInstance(FooArgument.class).argsHandled);
 	}
 
+	/**
+	 * Tests that {@link ConsoleService#processArgs(String...)} does not result in
+	 * an infinite loop when a buggy {@link ConsoleArgument} forgets to remove its
+	 * handled argument from the list.
+	 */
+	@Test
+	public void testInfiniteLoopAvoidance() {
+		assertFalse(consoleService.getInstance(BrokenArgument.class).argsHandled);
+		consoleService.processArgs("--broken");
+		assertTrue(consoleService.getInstance(BrokenArgument.class).argsHandled);
+	}
+
 	/**
 	 * Tests the {@link OutputListener}-related API:
 	 * <ul>
@@ -225,6 +239,24 @@ public void handle(final LinkedList<String> args) {
 		}
 	}
 
+	@Plugin(type = ConsoleArgument.class)
+	public static class BrokenArgument extends AbstractConsoleArgument {
+
+		private boolean argsHandled;
+
+		public BrokenArgument() {
+			super(1, "--broken");
+		}
+
+		@Override
+		public void handle(final LinkedList<String> args) {
+			// NB: Does not shorten the list. This is an intentional "bug" which
+			// would naively result in an infinite loop. We want to test that
+			// the ConsoleService is at least minorly resilient to this problem.
+			argsHandled = true;
+		}
+	}
+
 	private static class OutputTracker implements OutputListener {
 
 		private final Collection<OutputEvent> events;