Opened 3 years ago
Closed 3 years ago
#2092 closed defect (fixed)
NPE in RouterContext lock
Reported by: | str4d | Owned by: | zzz |
---|---|---|---|
Priority: | major | Milestone: | 0.9.33 |
Component: | router/general | Version: | 0.9.31 |
Keywords: | I2P Android | Cc: | |
Parent Tickets: | Sensitive: | no |
Description
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(); ...
Subtickets
Change History (6)
comment:1 Changed 3 years ago by
comment:2 Changed 3 years ago by
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.
comment:3 Changed 3 years ago by
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?
comment:4 Changed 3 years ago by
I'm referring to line 314-315 in the private constructor of I2PAppContext - that's where the leak happens.
comment:5 Changed 3 years ago by
Milestone: | undecided → 0.9.33 |
---|---|
Owner: | set to zzz |
Priority: | minor → major |
Status: | new → accepted |
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
comment:6 Changed 3 years ago by
Resolution: | → fixed |
---|---|
Status: | accepted → closed |
Cleaned up to also delay-store to the list of contexts in RouterContext?.
In 92efdb0b5059335ff04dd1fc44cc2baa207aeda0 0.9.32-9
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.