From 5a50887a4b92f18d8f33e6472cbb9bbdc9631499 Mon Sep 17 00:00:00 2001 From: Dawid Weiss Date: Thu, 18 May 2017 16:38:53 +0200 Subject: [PATCH] LUCENE-7800: Remove code that potentially rethrows checked exceptions from methods that don't declare them ("sneaky throw" hack). --- lucene/CHANGES.txt | 6 + .../tartarus/snowball/SnowballProgram.java | 20 ++- .../apache/lucene/util/AttributeFactory.java | 39 +++--- .../expressions/js/JavascriptCompiler.java | 131 +++++++++--------- 4 files changed, 94 insertions(+), 102 deletions(-) diff --git a/lucene/CHANGES.txt b/lucene/CHANGES.txt index 404e923d5a2..f309334b41d 100644 --- a/lucene/CHANGES.txt +++ b/lucene/CHANGES.txt @@ -97,6 +97,12 @@ Other ======================= Lucene 6.7.0 ======================= +Other + +* LUCENE-7800: Remove code that potentially rethrows checked exceptions + from methods that don't declare them ("sneaky throw" hack). (Robert Muir, + Uwe Schindler, Dawid Weiss) + ======================= Lucene 6.6.0 ======================= New Features diff --git a/lucene/analysis/common/src/java/org/tartarus/snowball/SnowballProgram.java b/lucene/analysis/common/src/java/org/tartarus/snowball/SnowballProgram.java index bfc8ec0b1c6..c31065fa7a7 100644 --- a/lucene/analysis/common/src/java/org/tartarus/snowball/SnowballProgram.java +++ b/lucene/analysis/common/src/java/org/tartarus/snowball/SnowballProgram.java @@ -31,6 +31,8 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. package org.tartarus.snowball; +import java.lang.reflect.UndeclaredThrowableException; + import org.apache.lucene.util.ArrayUtil; /** @@ -313,8 +315,10 @@ public abstract class SnowballProgram { boolean res = false; try { res = (boolean) w.method.invokeExact(this); + } catch (Error | RuntimeException e) { + throw e; } catch (Throwable e) { - rethrow(e); + throw new UndeclaredThrowableException(e); } cursor = c + w.s_size; if (res) return w.result; @@ -376,8 +380,10 @@ public abstract class SnowballProgram { boolean res = false; try { res = (boolean) w.method.invokeExact(this); + } catch (Error | RuntimeException e) { + throw e; } catch (Throwable e) { - rethrow(e); + throw new UndeclaredThrowableException(e); } cursor = c - w.s_size; if (res) return w.result; @@ -485,15 +491,5 @@ extern void debug(struct SN_env * z, int number, int line_count) printf("'\n"); } */ - - // Hack to rethrow unknown Exceptions from {@link MethodHandle#invoke}: - private static void rethrow(Throwable t) { - SnowballProgram.rethrow0(t); - } - - @SuppressWarnings("unchecked") - private static void rethrow0(Throwable t) throws T { - throw (T) t; - } }; diff --git a/lucene/core/src/java/org/apache/lucene/util/AttributeFactory.java b/lucene/core/src/java/org/apache/lucene/util/AttributeFactory.java index 6c3d6dbfafe..70ccadf444a 100644 --- a/lucene/core/src/java/org/apache/lucene/util/AttributeFactory.java +++ b/lucene/core/src/java/org/apache/lucene/util/AttributeFactory.java @@ -20,6 +20,7 @@ package org.apache.lucene.util; import java.lang.invoke.MethodHandle; import java.lang.invoke.MethodHandles; import java.lang.invoke.MethodType; +import java.lang.reflect.UndeclaredThrowableException; /** * An AttributeFactory creates instances of {@link AttributeImpl}s. @@ -28,8 +29,14 @@ public abstract class AttributeFactory { /** * Returns an {@link AttributeImpl} for the supplied {@link Attribute} interface class. + * + * @throws UndeclaredThrowableException A wrapper runtime exception thrown if the + * constructor of the attribute class throws a checked exception. + * Note that attributes should not throw or declare + * checked exceptions; this may be verified and fail early in the future. */ - public abstract AttributeImpl createAttributeInstance(Class attClass); + public abstract AttributeImpl createAttributeInstance(Class attClass) + throws UndeclaredThrowableException; /** * Returns a correctly typed {@link MethodHandle} for the no-arg ctor of the given class. @@ -61,17 +68,18 @@ public abstract class AttributeFactory { }; DefaultAttributeFactory() {} - + @Override public AttributeImpl createAttributeInstance(Class attClass) { try { return (AttributeImpl) constructors.get(attClass).invokeExact(); - } catch (Throwable t) { - rethrow(t); - throw new AssertionError(); + } catch (Error | RuntimeException e) { + throw e; + } catch (Throwable e) { + throw new UndeclaredThrowableException(e); } } - + private Class findImplClass(Class attClass) { try { return Class.forName(attClass.getName() + "Impl", true, attClass.getClassLoader()).asSubclass(AttributeImpl.class); @@ -138,23 +146,12 @@ public abstract class AttributeFactory { protected A createInstance() { try { return (A) constr.invokeExact(); - } catch (Throwable t) { - rethrow(t); - throw new AssertionError(); + } catch (Error | RuntimeException e) { + throw e; + } catch (Throwable e) { + throw new UndeclaredThrowableException(e); } } }; } - - // Hack to rethrow unknown Exceptions from {@link MethodHandle#invoke}: - // TODO: remove the impl in test-framework, this one is more elegant :-) - static void rethrow(Throwable t) { - AttributeFactory.rethrow0(t); - } - - @SuppressWarnings("unchecked") - private static void rethrow0(Throwable t) throws T { - throw (T) t; - } - } \ No newline at end of file diff --git a/lucene/expressions/src/java/org/apache/lucene/expressions/js/JavascriptCompiler.java b/lucene/expressions/src/java/org/apache/lucene/expressions/js/JavascriptCompiler.java index 87e41c0c953..465d43c2130 100644 --- a/lucene/expressions/src/java/org/apache/lucene/expressions/js/JavascriptCompiler.java +++ b/lucene/expressions/src/java/org/apache/lucene/expressions/js/JavascriptCompiler.java @@ -188,15 +188,20 @@ public final class JavascriptCompiler { private Expression compileExpression(ClassLoader parent) throws ParseException { final Map externalsMap = new LinkedHashMap<>(); final ClassWriter classWriter = new ClassWriter(ClassWriter.COMPUTE_FRAMES | ClassWriter.COMPUTE_MAXS); - - generateClass(getAntlrParseTree(), classWriter, externalsMap); - + try { + generateClass(getAntlrParseTree(), classWriter, externalsMap); + final Class evaluatorClass = new Loader(parent) .define(COMPILED_EXPRESSION_CLASS, classWriter.toByteArray()); final Constructor constructor = evaluatorClass.getConstructor(String.class, String[].class); return constructor.newInstance(sourceText, externalsMap.keySet().toArray(new String[externalsMap.size()])); + } catch (RuntimeException re) { + if (re.getCause() instanceof ParseException) { + throw (ParseException)re.getCause(); + } + throw re; } catch (ReflectiveOperationException exception) { throw new IllegalStateException("An internal error occurred attempting to compile the expression (" + sourceText + ").", exception); } @@ -209,20 +214,13 @@ public final class JavascriptCompiler { * @throws ParseException on failure to parse */ private ParseTree getAntlrParseTree() throws ParseException { - try { - final ANTLRInputStream antlrInputStream = new ANTLRInputStream(sourceText); - final JavascriptErrorHandlingLexer javascriptLexer = new JavascriptErrorHandlingLexer(antlrInputStream); - javascriptLexer.removeErrorListeners(); - final JavascriptParser javascriptParser = new JavascriptParser(new CommonTokenStream(javascriptLexer)); - javascriptParser.removeErrorListeners(); - javascriptParser.setErrorHandler(new JavascriptParserErrorStrategy()); - return javascriptParser.compile(); - } catch (RuntimeException re) { - if (re.getCause() instanceof ParseException) { - throw (ParseException)re.getCause(); - } - throw re; - } + final ANTLRInputStream antlrInputStream = new ANTLRInputStream(sourceText); + final JavascriptErrorHandlingLexer javascriptLexer = new JavascriptErrorHandlingLexer(antlrInputStream); + javascriptLexer.removeErrorListeners(); + final JavascriptParser javascriptParser = new JavascriptParser(new CommonTokenStream(javascriptLexer)); + javascriptParser.removeErrorListeners(); + javascriptParser.setErrorHandler(new JavascriptParserErrorStrategy()); + return javascriptParser.compile(); } /** @@ -291,51 +289,56 @@ public final class JavascriptCompiler { boolean parens = ctx.LP() != null && ctx.RP() != null; Method method = parens ? functions.get(text) : null; - if (method != null) { - int arity = method.getParameterTypes().length; - - if (arguments != arity) { - throwChecked(new ParseException( - "Invalid expression '" + sourceText + "': Expected (" + - arity + ") arguments for function call (" + text + "), but found (" + arguments + ").", - ctx.start.getStartIndex())); - } - - typeStack.push(Type.DOUBLE_TYPE); - - for (int argument = 0; argument < arguments; ++argument) { - visit(ctx.expression(argument)); - } - - typeStack.pop(); - - gen.invokeStatic(Type.getType(method.getDeclaringClass()), - org.objectweb.asm.commons.Method.getMethod(method)); - - gen.cast(Type.DOUBLE_TYPE, typeStack.peek()); - } else if (!parens || arguments == 0 && text.contains(".")) { - int index; - - text = normalizeQuotes(ctx.getText()); - - if (externalsMap.containsKey(text)) { - index = externalsMap.get(text); + try { + if (method != null) { + int arity = method.getParameterTypes().length; + + if (arguments != arity) { + throw new ParseException( + "Invalid expression '" + sourceText + "': Expected (" + + arity + ") arguments for function call (" + text + "), but found (" + arguments + ").", + ctx.start.getStartIndex()); + } + + typeStack.push(Type.DOUBLE_TYPE); + + for (int argument = 0; argument < arguments; ++argument) { + visit(ctx.expression(argument)); + } + + typeStack.pop(); + + gen.invokeStatic(Type.getType(method.getDeclaringClass()), + org.objectweb.asm.commons.Method.getMethod(method)); + + gen.cast(Type.DOUBLE_TYPE, typeStack.peek()); + } else if (!parens || arguments == 0 && text.contains(".")) { + int index; + + text = normalizeQuotes(ctx.getText()); + + if (externalsMap.containsKey(text)) { + index = externalsMap.get(text); + } else { + index = externalsMap.size(); + externalsMap.put(text, index); + } + + gen.loadArg(0); + gen.push(index); + gen.arrayLoad(FUNCTION_VALUES_TYPE); + gen.invokeVirtual(FUNCTION_VALUES_TYPE, DOUBLE_VAL_METHOD); + gen.cast(Type.DOUBLE_TYPE, typeStack.peek()); } else { - index = externalsMap.size(); - externalsMap.put(text, index); + throw new ParseException("Invalid expression '" + sourceText + "': Unrecognized function call (" + + text + ").", ctx.start.getStartIndex()); } - - gen.loadArg(0); - gen.push(index); - gen.arrayLoad(FUNCTION_VALUES_TYPE); - gen.invokeVirtual(FUNCTION_VALUES_TYPE, DOUBLE_VAL_METHOD); - gen.cast(Type.DOUBLE_TYPE, typeStack.peek()); - } else { - throwChecked(new ParseException("Invalid expression '" + sourceText + "': Unrecognized function call (" + - text + ").", ctx.start.getStartIndex())); + return null; + } catch (ParseException e) { + // The API doesn't allow checked exceptions here, so propagate up the stack. This is unwrapped + // in getAntlrParseTree. + throw new RuntimeException(e); } - - return null; } @Override @@ -623,16 +626,6 @@ public final class JavascriptCompiler { throw new IllegalStateException("Invalid expected type: " + typeStack.peek()); } } - - /** Needed to throw checked ParseException in this visitor (that does not allow it). */ - private void throwChecked(Throwable t) { - this.throwChecked0(t); - } - - @SuppressWarnings("unchecked") - private void throwChecked0(Throwable t) throws T { - throw (T) t; - } }.visit(parseTree); gen.returnValue();