NPE in RouterContext lock

[str4d]

This is a strange one. I have the following traceback across Android 5.0, 5.1 and 6.0:

java.lang.NullPointerException: 
  at net.i2p.router.RouterContext.initializeClock (RouterContext.java:491)
  at net.i2p.I2PAppContext.clock (I2PAppContext.java:830)
  at net.i2p.util.LogManager.loadConfig (LogManager.java:355)
  at net.i2p.util.LogManager.setConfig (LogManager.java:264)
  at net.i2p.util.LogManager.<init> (LogManager.java:146)
  at net.i2p.I2PAppContext.initializeLogManager (I2PAppContext.java:722)
  at net.i2p.I2PAppContext.logManager (I2PAppContext.java:715)
  at net.i2p.i2ptunnel.TunnelControllerGroup.<init> (TunnelControllerGroup.java:112)
  at net.i2p.i2ptunnel.TunnelControllerGroup.getInstance (TunnelControllerGroup.java:89)
  at net.i2p.android.i2ptunnel.TunnelListFragment.initTunnels (TunnelListFragment.java:166)
  at net.i2p.android.i2ptunnel.TunnelListFragment.updateState (TunnelListFragment.java:160)
  at net.i2p.android.i2ptunnel.TunnelListFragment$1.onReceive (TunnelListFragment.java:146)
  at android.support.v4.content.LocalBroadcastManager.executePendingBroadcasts (LocalBroadcastManager.java:297)
  at android.support.v4.content.LocalBroadcastManager.access$000 (LocalBroadcastManager.java:46)
  at android.support.v4.content.LocalBroadcastManager$1.handleMessage (LocalBroadcastManager.java:116)
  at android.os.Handler.dispatchMessage (Handler.java:102)
  at android.os.Looper.loop (Looper.java:148)
  at android.app.ActivityThread.main (ActivityThread.java:5468)
  at java.lang.reflect.Method.invoke (Native Method)
  at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run (ZygoteInit.java:781)
  at com.android.internal.os.ZygoteInit.main (ZygoteInit.java:671)

The code in question:

    @Override
    protected void initializeClock() {
        synchronized (_lock1) { <------------- NPE
            if (_clock == null) {
                RouterClock rc = new RouterClock(this);
                rc.start();
                _clock = rc;
            }
            _clockInitialized = true;
        }
    }

Looks like it is complaining that _lock1 is null, but the only other place it is used is its definition:

public class RouterContext extends I2PAppContext {

...

    private final Object _lock1 = new Object(), _lock2 = new Object(), _lock3 = new Object();

...

[Zlatin Balevsky]

I think the key to solving this is the fact that ZygoteInit uses reflection to instantiate whatever it is instantiating. A workaround would be to a) explicitly load the RouterContext class beforehand b) move the _lock* initialization above the _logManager definition.

A deep dive on ZygoteInit is necessary, but I'm pretty sure this would not happen on non-Android.

[Zlatin Balevsky]

Actually nevermind last comment, don't think reflection has anything to do with it. I think the issue is the Double-Checked Locking in I2PAppContext.getGlobalContext(). The sequence of events is:

Thread A : creates RouterContext, calls super (final fields get initialized at the beginning of a constructor, but not before a call to super afaik) and writes the not-yet-fully constructed object to _globalAppContext. It gets preempted, while still holding the lock in the constructor.

Thread B: calls I2PAppContext.getGlobalContext(), sees the field is not null and returns not fully constructed object.

[zzz]

Huh? Finals are final if the constructor is well-behaved, and they all are.

The constructor is complete before it's written to _globalAppContext.

str4d, you said you changed API to 25 as an experiment and just discovered lots of problems, why not change it back and be done?

[Zlatin Balevsky]

I'm referring to line 314-315 in the private constructor of I2PAppContext - that's where the leak happens.

[zzz]

zab is exactly right, thanks for the explanation, sorry I'm slow.

as discussed on IRC, something like this? compile-tested only…

#
# old_revision [b6c6d3d49273f1eebab9b63a91f405a47ed66946]
#
# patch "core/java/src/net/i2p/I2PAppContext.java"
#  from [168bfb9e7781e4338ed0148653112c294e392d9d]
#    to [59a6feaee7c24dae70965f6dd4877d2ab911f643]
# 
# patch "router/java/src/net/i2p/router/Router.java"
#  from [36fe2c33313978d9f83156c3f8681bd8e86379f1]
#    to [5e95a733a2fbc7eccb09000413e346f60998b69d]
# 
# patch "router/java/src/net/i2p/router/RouterContext.java"
#  from [3374fd2e0916b46a13512ac8ba127967fcc2fa19]
#    to [ee1b861dbc7c051365f66f6a1df70a2cfcc0261d]
#
============================================================
--- core/java/src/net/i2p/I2PAppContext.java	168bfb9e7781e4338ed0148653112c294e392d9d
+++ core/java/src/net/i2p/I2PAppContext.java	59a6feaee7c24dae70965f6dd4877d2ab911f643
@@ -149,7 +149,29 @@ public class I2PAppContext {
         return _globalAppContext; 
     }
     
+
     /**
+     * Sets the default context, unless there is one already.
+     * NOT a public API, for use by Router only, NOT for external use.
+     *
+     * @param ctx context constructed with doInit = false
+     * @return success (false if previously set)
+     * @since 0.9.33
+     */
+    public static boolean setGlobalContext(I2PAppContext ctx) {
+        synchronized (I2PAppContext.class) {
+            if (_globalAppContext == null) {
+                _globalAppContext = ctx;
+                return true;
+            } else {
+                System.out.println("Warning - New context not replacing old one, you now have a second one");
+                (new Exception("I did it")).printStackTrace();
+                return false;
+            }
+        }
+    }
+
+    /**
      * Pull the default context, WITHOUT creating a new one.
      * Use this in static methods used early in router initialization,
      * where creating a context messes things up.
@@ -190,10 +212,13 @@ public class I2PAppContext {
      * additional resources and threads, and may be the cause of logging
      * problems or hard-to-isolate bugs.
      *
+     * NOT a public API, for use by RouterContext only, NOT for external use.
+     *
      * @param doInit should this context be used as the global one (if necessary)?
      *               Will only apply if there is no global context now.
+     * @since public since 0.9.33, NOT for external use
      */
-    private I2PAppContext(boolean doInit, Properties envProps) {
+    public I2PAppContext(boolean doInit, Properties envProps) {
       synchronized (I2PAppContext.class) { 
         _overrideProps = new I2PProperties();
         if (envProps != null)
@@ -311,12 +336,9 @@ public class I2PAppContext {
         ******/
 
         if (doInit) {
-            if (_globalAppContext == null) {
-                _globalAppContext = this;
-            } else {
-                System.out.println("Warning - New context not replacing old one, you now have a second one");
-                (new Exception("I did it")).printStackTrace();
-            }
+            // Bad practice, sets a static field to this in constructor.
+            // doInit will be false when instantiated via Router.
+            setGlobalContext(this);
         }
       } // synch
     }
============================================================
--- router/java/src/net/i2p/router/Router.java	36fe2c33313978d9f83156c3f8681bd8e86379f1
+++ router/java/src/net/i2p/router/Router.java	5e95a733a2fbc7eccb09000413e346f60998b69d
@@ -23,6 +23,7 @@ import gnu.getopt.Getopt;
 
 import gnu.getopt.Getopt;
 
+import net.i2p.I2PAppContext;
 import net.i2p.client.impl.I2PSessionImpl;
 import net.i2p.crypto.SigUtil;
 import net.i2p.data.Base64;
@@ -308,7 +309,8 @@ public class Router implements RouterClo
         // i2p.dir.log defaults to i2p.dir.router
         // i2p.dir.pid defaults to i2p.dir.router
         // i2p.dir.base defaults to user.dir == $CWD
-        _context = new RouterContext(this, envProps);
+        _context = new RouterContext(this, envProps, false);
+        I2PAppContext.setGlobalContext(_context);
         _eventLog = new EventLog(_context, new File(_context.getRouterDir(), EVENTLOG));
 
         // This is here so that we can get the directory location from the context
============================================================
--- router/java/src/net/i2p/router/RouterContext.java	3374fd2e0916b46a13512ac8ba127967fcc2fa19
+++ router/java/src/net/i2p/router/RouterContext.java	ee1b861dbc7c051365f66f6a1df70a2cfcc0261d
@@ -84,7 +84,20 @@ public class RouterContext extends I2PAp
      *  Caller MUST call initAll() after instantiation.
      */
     public RouterContext(Router router, Properties envProps) { 
-        super(filterProps(envProps));
+        this(router, envProps, true);
+    }
+
+    /**
+     *  Caller MUST call initAll() after instantiation.
+     *  NOT a public API, for use by Router only, NOT for external use.
+     *
+     *  @param doInit should this context be used as the global one (if necessary)?
+     *                Will only apply if there is no global context now.
+     *                If false, caller should call I2PAppContext.setGlobalContext() afterwards.
+     *  @since 0.9.33
+     */
+    RouterContext(Router router, Properties envProps, boolean doInit) { 
+        super(doInit, filterProps(envProps));
         _router = router;
         // Disabled here so that the router can get a context and get the
         // directory locations from it, to do an update, without having

[zzz]

Cleaned up to also delay-store to the list of contexts in RouterContext?.

In 92efdb0b5059335ff04dd1fc44cc2baa207aeda0 0.9.32-9