diff --git a/.travis.yml b/.travis.yml index 9b59276094..605a9a8665 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,23 +1,24 @@ -# Licensed to the Apache Software Foundation (ASF) under one or more -# contributor license agreements. See the NOTICE file distributed with -# this work for additional information regarding copyright ownership. -# The ASF licenses this file to You under the Apache License, Version 2.0 -# (the "License"); you may not use this file except in compliance with -# the License. You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. - -language: java -sudo: false - -jdk: - - oraclejdk8 - -after_success: - - mvn -Ddoclint:none clean cobertura:cobertura coveralls:report +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +language: java +sudo: false + +jdk: + - openjdk8 + - oraclejdk8 + +after_success: + - mvn -Ddoclint:none clean cobertura:cobertura coveralls:report diff --git a/src/changes/changes.xml b/src/changes/changes.xml index b53126f99a..7246186d48 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -63,6 +63,7 @@ The type attribute can be add,update,fix,remove. + Race conditions on static fields in BranchHandle and InstructionHandle. diff --git a/src/main/java/org/apache/bcel/generic/BranchHandle.java b/src/main/java/org/apache/bcel/generic/BranchHandle.java index 8c1b478750..94627f1198 100644 --- a/src/main/java/org/apache/bcel/generic/BranchHandle.java +++ b/src/main/java/org/apache/bcel/generic/BranchHandle.java @@ -40,28 +40,10 @@ private BranchHandle(final BranchInstruction i) { bi = i; } - /** Factory methods. + /** Factory method. */ - private static BranchHandle bh_list = null; // List of reusable handles - - static BranchHandle getBranchHandle( final BranchInstruction i ) { - if (bh_list == null) { - return new BranchHandle(i); - } - final BranchHandle bh = bh_list; - bh_list = (BranchHandle) bh.getNext(); - bh.setInstruction(i); - return bh; - } - - - /** Handle adds itself to the list of resuable handles. - */ - @Override - protected void addHandle() { - super.setNext(bh_list); - bh_list = this; + return new BranchHandle(i); } diff --git a/src/main/java/org/apache/bcel/generic/InstructionHandle.java b/src/main/java/org/apache/bcel/generic/InstructionHandle.java index d3994ce633..f91b0252d9 100644 --- a/src/main/java/org/apache/bcel/generic/InstructionHandle.java +++ b/src/main/java/org/apache/bcel/generic/InstructionHandle.java @@ -113,19 +113,10 @@ public Instruction swapInstruction( final Instruction i ) { setInstruction(i); } - private static InstructionHandle ih_list = null; // List of reusable handles - - /** Factory method. */ static InstructionHandle getInstructionHandle( final Instruction i ) { - if (ih_list == null) { - return new InstructionHandle(i); - } - final InstructionHandle ih = ih_list; - ih_list = ih.next; - ih.setInstruction(i); - return ih; + return new InstructionHandle(i); } @@ -162,16 +153,8 @@ void setPosition( final int pos ) { } - /** Overridden in BranchHandle - */ - protected void addHandle() { - next = ih_list; - ih_list = this; - } - - /** - * Delete contents, i.e., remove user access and make handle reusable. + * Delete contents, i.e., remove user access. */ void dispose() { next = prev = null; @@ -180,7 +163,6 @@ void dispose() { i_position = -1; attributes = null; removeAllTargeters(); - addHandle(); } diff --git a/src/main/java/org/apache/bcel/util/ClassLoader.java b/src/main/java/org/apache/bcel/util/ClassLoader.java index d66d59538f..f4ba5c7bec 100644 --- a/src/main/java/org/apache/bcel/util/ClassLoader.java +++ b/src/main/java/org/apache/bcel/util/ClassLoader.java @@ -21,7 +21,7 @@ import java.io.IOException; import java.util.Hashtable; -import org.apache.bcel.Constants; +import org.apache.bcel.Const; import org.apache.bcel.classfile.ClassParser; import org.apache.bcel.classfile.ConstantClass; import org.apache.bcel.classfile.ConstantPool; @@ -183,9 +183,9 @@ protected JavaClass createClass( final String class_name ) { // Adapt the class name to the passed value final ConstantPool cp = clazz.getConstantPool(); final ConstantClass cl = (ConstantClass) cp.getConstant(clazz.getClassNameIndex(), - Constants.CONSTANT_Class); + Const.CONSTANT_Class); final ConstantUtf8 name = (ConstantUtf8) cp.getConstant(cl.getNameIndex(), - Constants.CONSTANT_Utf8); + Const.CONSTANT_Utf8); name.setBytes(class_name.replace('.', '/')); return clazz; } diff --git a/src/test/java/org/apache/bcel/HandleTestCase.java b/src/test/java/org/apache/bcel/HandleTestCase.java new file mode 100644 index 0000000000..c964360c2d --- /dev/null +++ b/src/test/java/org/apache/bcel/HandleTestCase.java @@ -0,0 +1,138 @@ +package org.apache.bcel; + +import org.apache.bcel.generic.GOTO; +import org.apache.bcel.generic.ILOAD; +import org.apache.bcel.generic.InstructionHandle; +import org.apache.bcel.generic.InstructionList; +import org.apache.bcel.generic.NOP; +import org.junit.Test; + +import junit.framework.AssertionFailedError; + +/** + * Test for https://issues.apache.org/jira/browse/BCEL-267 "Race conditions on + * static fields in BranchHandle and InstructionHandle". + */ +public class HandleTestCase { + + static Throwable exception; + static final int MAXI = 100; + static final int MAXJ = 1000; + + /** + * Asserts that branch handles can be added an instruction list, without + * corrupting the list. + */ + static void branchHandles() { + for (int i = 0; i < MAXI; i++) { + final InstructionList list = new InstructionList(); + final InstructionHandle start = list.append(new NOP()); + try { + for (int j = 0; j < MAXJ; j++) { + list.append(new GOTO(start)); + } + final InstructionHandle[] instructionHandles = list.getInstructionHandles(); + for (int j = 0; j < instructionHandles.length; j++) { + final InstructionHandle handle = instructionHandles[j]; + if (j > 0) { + checkLinkage(handle, j); + if (start != ((GOTO) handle.getInstruction()).getTarget()) { + final AssertionFailedError error = new AssertionFailedError( + "unexpected instruction at index " + j); + exception = error; + throw error; + } + } + } + if (exception != null) { + return; + } + } catch (final NullPointerException e) { + System.out.println("NPE at i=" + i); + exception = e; + throw e; + } + list.dispose(); // this initializes caching of unused instruction handles + } + } + + /** + * Assert that opposite next/prev pairs always match. + */ + static void checkLinkage(final InstructionHandle ih, final int index) { + final InstructionHandle prev = ih.getPrev(); + final InstructionHandle next = ih.getNext(); + if ((prev != null && prev.getNext() != ih) || (next != null && next.getPrev() != ih)) { + final AssertionFailedError error = new AssertionFailedError("corrupt instruction list at index " + index); + exception = error; + throw error; + } + } + + /** + * Asserts that instruction handles can be added an instruction list, without + * corrupting the list. + */ + static void handles() { + for (int i = 0; i < MAXI; i++) { + final InstructionList list = new InstructionList(); + try { + for (int j = 0; j < MAXJ; j++) { + list.append(new ILOAD(j)); + } + final InstructionHandle[] instructionHandles = list.getInstructionHandles(); + for (int j = 0; j < instructionHandles.length; j++) { + final InstructionHandle handle = instructionHandles[j]; + checkLinkage(handle, j); + if (j != ((ILOAD) handle.getInstruction()).getIndex()) { + final AssertionFailedError error = new AssertionFailedError("unexpected instruction at index " + j); + exception = error; + throw error; + } + } + if (exception != null) { + return; + } + } catch (final NullPointerException e) { + System.out.println("NPE at i=" + i); + exception = e; + throw e; + } + list.dispose(); // this initializes caching of unused instruction handles + } + } + + /** + * Concurrently run the given runnable in two threads. + */ + private void perform(final Runnable r) throws Throwable { + exception = null; + final Thread t1 = new Thread(r); + final Thread t2 = new Thread(r); + t1.start(); + t2.start(); + t1.join(); + t2.join(); + if (exception != null) { + throw exception; + } + } + + /** + * Assert that two independent instruction lists can be modified concurrently. + * Here: inserting branch instructions. + */ + @Test + public void testBranchHandle() throws Throwable { + perform(HandleTestCase::branchHandles); + } + + /** + * Assert that two independent instruction lists can be modified concurrently. + * Here: inserting regular instructions. + */ + @Test + public void testInstructionHandle() throws Throwable { + perform(HandleTestCase::handles); + } +} \ No newline at end of file diff --git a/src/test/java/org/apache/bcel/util/BCELifierTestCase.java b/src/test/java/org/apache/bcel/util/BCELifierTestCase.java index 7ec313d459..ceb7213c14 100644 --- a/src/test/java/org/apache/bcel/util/BCELifierTestCase.java +++ b/src/test/java/org/apache/bcel/util/BCELifierTestCase.java @@ -63,7 +63,7 @@ private void testClassOnPath(final String javaClass) throws Exception { final BCELifier bcelifier = new BCELifier(java_class, fos); bcelifier.start(); } - exec(workDir, "javac", "-cp", "classes", outfile.getName()); + exec(workDir, "javac", "-cp", "classes", outfile.getName(), "-source", "1.8", "-target", "1.8"); exec(workDir, "java", "-cp", "." + File.pathSeparator + "classes", outfile.getName().replace(".java", "")); final String output = exec(workDir, "javap", "-p", "-c", infile.getName()); assertEquals(canonHashRef(initial), canonHashRef(output));