BUG-893: fix loaderClassPaths safety

This fixes thread safety of loaderClassPaths, as there was a
TOCTOU error. Furthermore this actually populates the map, such that we
do not continue to add classloaders.

Drive-bys include final keywords and cleanup on utility classes.

Change-Id: I3c96a4e98b242d34d7036bcb81f71f36ff88223c
Signed-off-by: Robert Varga <rovarga@cisco.com>

diff --git a/code-generator/binding-generator-impl/src/main/java/org/opendaylight/yangtools/sal/binding/generator/util/JavassistUtils.java b/code-generator/binding-generator-impl/src/main/java/org/opendaylight/yangtools/sal/binding/generator/util/JavassistUtils.java
index 38e43e9cb1dd4f849ce7d1fbce317e7561a6be5e..b0d1fcb47a8a634a21764d5e0357d9b8ac62a6aa 100644 (file)
--- a/code-generator/binding-generator-impl/src/main/java/org/opendaylight/yangtools/sal/binding/generator/util/JavassistUtils.java
+++ b/code-generator/binding-generator-impl/src/main/java/org/opendaylight/yangtools/sal/binding/generator/util/JavassistUtils.java
@@ -7,21 +7,24 @@
  */
 package org.opendaylight.yangtools.sal.binding.generator.util;
 
-import javassist.CtClass;
-import javassist.CtMethod;
-import javassist.ClassPool;
-
+import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collection;
+import java.util.List;
+import java.util.Map;
+import java.util.WeakHashMap;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
 
+import javassist.ClassClassPath;
 import javassist.ClassPath;
+import javassist.ClassPool;
+import javassist.CtClass;
 import javassist.CtField;
+import javassist.CtMethod;
+import javassist.LoaderClassPath;
 import javassist.Modifier;
 import javassist.NotFoundException;
-import javassist.LoaderClassPath;
-import javassist.ClassClassPath;
-
-import java.util.concurrent.locks.Lock;
-import java.util.concurrent.locks.ReentrantLock;
 
 import org.eclipse.xtext.xbase.lib.Conversions;
 import org.eclipse.xtext.xbase.lib.Exceptions;
@@ -30,24 +33,15 @@ import org.slf4j.LoggerFactory;
 
 import com.google.common.base.Preconditions;
 
-import java.util.ArrayList;
-import java.util.Collection;
-import java.util.List;
-import java.util.Map;
-import java.util.WeakHashMap;
-import java.util.Collections;
-
 public class JavassistUtils {
-
     private static final Logger LOG = LoggerFactory.getLogger(JavassistUtils.class);
 
-    private final Map<ClassLoader, LoaderClassPath> loaderClassPaths = Collections
-            .synchronizedMap(new WeakHashMap<ClassLoader, LoaderClassPath>());
-    private ClassPool classPool;
+    private final Map<ClassLoader, ClassPath> loaderClassPaths = new WeakHashMap<>();
     private final Lock lock = new ReentrantLock();
+    private final ClassPool classPool;
 
-    public JavassistUtils(ClassPool pool) {
-        classPool = pool;
+    public JavassistUtils(final ClassPool pool) {
+        classPool = Preconditions.checkNotNull(pool);
     }
 
     public Lock getLock() {
@@ -98,7 +92,7 @@ public class JavassistUtils {
         }
     }
 
-    public void implementMethodsFrom(CtClass target, CtClass source, MethodGenerator function1) {
+    public void implementMethodsFrom(final CtClass target, final CtClass source, final MethodGenerator function1) {
         try {
             for (CtMethod method : source.getMethods()) {
                 if (method.getDeclaringClass() == source) {
@@ -112,13 +106,13 @@ public class JavassistUtils {
         }
     }
 
-    public CtClass createClass(String fqn, ClassGenerator cls) {
+    public CtClass createClass(final String fqn, final ClassGenerator cls) {
         CtClass target = classPool.makeClass(fqn);
         cls.process(target);
         return target;
     }
 
-    public CtClass createClass(String fqn, CtClass superInterface, ClassGenerator cls) {
+    public CtClass createClass(final String fqn, final CtClass superInterface, final ClassGenerator cls) {
         CtClass target = classPool.makeClass(fqn);
         implementsType(target, superInterface);
         cls.process(target);
@@ -181,15 +175,18 @@ public class JavassistUtils {
         }
     }
 
-    public void appendClassLoaderIfMissing(ClassLoader loader) {
-        if (loaderClassPaths.containsKey(loader)) {
-            return;
+    public synchronized void appendClassLoaderIfMissing(final ClassLoader loader) {
+        // FIXME: this works as long as the ClassPool is not shared between instances of this class
+        //        How is synchronization across multiple instances done? The ClassPool itself just
+        //        keeps on adding the loaders and does not check for duplicates!
+        if (!loaderClassPaths.containsKey(loader)) {
+            final ClassPath ctLoader = new LoaderClassPath(loader);
+            classPool.appendClassPath(ctLoader);
+            loaderClassPaths.put(loader, ctLoader);
         }
-        ClassPath ctLoader = new LoaderClassPath(loader);
-        classPool.appendClassPath(ctLoader);
     }
 
-    public void ensureClassLoader(Class<?> child) {
+    public void ensureClassLoader(final Class<?> child) {
         appendClassLoaderIfMissing(child.getClassLoader());
     }
 }

diff --git a/code-generator/binding-generator-impl/src/main/java/org/opendaylight/yangtools/sal/binding/generator/util/XtendHelper.java b/code-generator/binding-generator-impl/src/main/java/org/opendaylight/yangtools/sal/binding/generator/util/XtendHelper.java
index bd57c8032f9d08f1a93d1d7d4706332e07cc7810..4809c509261c32e6e7830d24b4cbc19a49e1aa78 100644 (file)
--- a/code-generator/binding-generator-impl/src/main/java/org/opendaylight/yangtools/sal/binding/generator/util/XtendHelper.java
+++ b/code-generator/binding-generator-impl/src/main/java/org/opendaylight/yangtools/sal/binding/generator/util/XtendHelper.java
@@ -12,10 +12,13 @@ import java.util.List;
 import org.opendaylight.yangtools.yang.model.api.TypeDefinition;
 import org.opendaylight.yangtools.yang.model.api.type.UnionTypeDefinition;
 
-public class XtendHelper {
+public final class XtendHelper {
+    private XtendHelper() {
+        throw new UnsupportedOperationException("Utility class should not be instantiated");
+    }
 
     @SuppressWarnings({"rawtypes","unchecked"})
-    public static Iterable<TypeDefinition> getTypes(UnionTypeDefinition definition) {
-        return (Iterable<TypeDefinition>) (List) definition.getTypes();
+    public static Iterable<TypeDefinition> getTypes(final UnionTypeDefinition definition) {
+        return (List) definition.getTypes();
     }
 }

diff --git a/code-generator/binding-generator-impl/src/main/java/org/opendaylight/yangtools/sal/binding/generator/util/YangSchemaUtils.java b/code-generator/binding-generator-impl/src/main/java/org/opendaylight/yangtools/sal/binding/generator/util/YangSchemaUtils.java
index faff9918a54b2141805dcbe51589486fc85a6e69..bf6fcec33379460f4971412e3c7096739573991f 100644 (file)
--- a/code-generator/binding-generator-impl/src/main/java/org/opendaylight/yangtools/sal/binding/generator/util/YangSchemaUtils.java
+++ b/code-generator/binding-generator-impl/src/main/java/org/opendaylight/yangtools/sal/binding/generator/util/YangSchemaUtils.java
@@ -25,17 +25,14 @@ import org.opendaylight.yangtools.yang.model.api.SchemaPath;
 import org.opendaylight.yangtools.yang.model.api.TypeDefinition;
 import org.opendaylight.yangtools.yang.model.api.UnknownSchemaNode;
 
-public class YangSchemaUtils {
-
+public final class YangSchemaUtils {
     public static final String AUGMENT_IDENTIFIER = "augment-identifier";
 
-
-    public YangSchemaUtils() {
+    private YangSchemaUtils() {
         throw new UnsupportedOperationException("Helper class. Instantiation is prohibited");
     }
 
-
-    public static QName getAugmentationQName(AugmentationSchema augmentation) {
+    public static QName getAugmentationQName(final AugmentationSchema augmentation) {
         checkNotNull(augmentation, "Augmentation must not be null.");
         QName identifier = getAugmentationIdentifier(augmentation);
         if(identifier != null) {
@@ -47,13 +44,14 @@ public class YangSchemaUtils {
             namespace = ((NamespaceRevisionAware) augmentation).getNamespace();
             revision = ((NamespaceRevisionAware) augmentation).getRevision();
         }
-        if(namespace == null || revision == null)
-        for(DataSchemaNode child : augmentation.getChildNodes()) {
-            // Derive QName from child nodes
-            if(!child.isAugmenting()) {
-                namespace = child.getQName().getNamespace();
-                revision = child.getQName().getRevision();
-                break;
+        if(namespace == null || revision == null) {
+            for(DataSchemaNode child : augmentation.getChildNodes()) {
+                // Derive QName from child nodes
+                if(!child.isAugmenting()) {
+                    namespace = child.getQName().getNamespace();
+                    revision = child.getQName().getRevision();
+                    break;
+                }
             }
         }
         checkState(namespace != null, "Augmentation namespace must not be null");
@@ -62,7 +60,7 @@ public class YangSchemaUtils {
         return QName.create(namespace,revision, "foo_augment");
     }
 
-    public static QName getAugmentationIdentifier(AugmentationSchema augmentation) {
+    public static QName getAugmentationIdentifier(final AugmentationSchema augmentation) {
         for(UnknownSchemaNode extension : augmentation.getUnknownSchemaNodes()) {
             if(AUGMENT_IDENTIFIER.equals(extension.getNodeType().getLocalName())) {
                 return extension.getQName();
@@ -71,8 +69,7 @@ public class YangSchemaUtils {
         return null;
     }
 
-
-    public static TypeDefinition<?> findTypeDefinition(SchemaContext context, SchemaPath path) {
+    public static TypeDefinition<?> findTypeDefinition(final SchemaContext context, final SchemaPath path) {
         List<QName> arguments = path.getPath();
         QName first = arguments.get(0);
         QName typeQName = arguments.get(arguments.size() -1);