ensure TorService properly shuts down whenever the tor thread stops

[eighthave]

This unnests the try/finally block that was added in pull #59, and moves it to the main try block, where it now handles the whole shutdown procedure.

• Allow TorService to be used in a separate process #61 (comment)
• Destroying TorService a second time causes crash #57
• Prevent concurrent calls to Tor API #59

@grote @akwizgran

The intention of this is probably easier to see when the diff starts from before 09de98d in #59:

--- a/tor-android-binary/src/main/java/org/torproject/jni/TorService.java
+++ b/tor-android-binary/src/main/java/org/torproject/jni/TorService.java
@@ -7,6 +7,7 @@ import android.os.Binder;
 import android.os.FileObserver;
 import android.os.IBinder;
 import android.os.Process;
+import android.util.Log;
 
 import net.freehaven.tor.control.RawEventListener;
 import net.freehaven.tor.control.TorControlCommands;
@@ -24,6 +25,7 @@ import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.locks.ReentrantLock;
 
 import androidx.annotation.Nullable;
 import androidx.localbroadcastmanager.content.LocalBroadcastManager;
@@ -167,6 +166,12 @@ public class TorService extends Service {
 
     private TorControlConnection torControlConnection;
 
+    /**
+     * This lock must be acquired before calling createTorConfiguration() and
+     * held until mainConfigurationFree() has been called.
+     */
+    private static final ReentrantLock runLock = new ReentrantLock();
+
     private native String apiGetProviderVersion();
 
     private native boolean createTorConfiguration();

@@ -314,6 +320,11 @@ public class TorService extends Service {
                         httpTunnelPort = Integer.toString(8118);
                     }
 
+                    if (runLock.isLocked()) {
+                        Log.i(TAG, "Waiting for lock");
+                    }
+                    runLock.lock();
+                    Log.i(TAG, "Acquired lock");
                     createTorConfiguration();
                     ArrayList<String> lines = new ArrayList<>(Arrays.asList("tor", "--verify-config", // must always be here
                             "--RunAsDaemon", "0",
@@ -363,11 +374,16 @@ public class TorService extends Service {
                 } catch (IllegalStateException | IllegalArgumentException | InterruptedException e) {
                     e.printStackTrace();
                     broadcastError(context, e);
+                } finally {
                     broadcastStatus(context, STATUS_STOPPING);
+                    mainConfigurationFree();
+                    Log.i(TAG, "Releasing lock");
+                    runLock.unlock();
                     TorService.this.stopSelf();
                 }
             }
         }.start();
+
     }
 
     @Override
@@ -377,7 +393,6 @@ public class TorService extends Service {
             torControlConnection.removeRawEventListener(startedEventListener);
         }
         shutdownTor(TorControlCommands.SIGNAL_SHUTDOWN);
-        mainConfigurationFree();
         broadcastStatus(TorService.this, STATUS_OFF);
     }
 

[eighthave]

Also, working on this made me think: what happens when TorService is given kill -9 from Android and the finally block isn't run? Will runLock still be locked?

[akwizgran]

Also, working on this made me think: what happens when TorService is given kill -9 from Android and the finally block isn't run? Will runLock still be locked?

The static lock is shared by TorService instances within the same process. kill -9 will kill the process, taking the static lock with it.

[syphyr]

After applying this commit, Orbot no longer shuts down

[syphyr]

12-21 20:31:28.753 8596 8596 W InputEventReceiver: Attempted to finish an input event but the input event receiver has already been disposed.
12-21 20:35:04.019 8596 8938 D Orbot : stopTor

[eighthave]

I believe this is the same as #67 or is it something different?

[syphyr]

I believe it is something different, because I did not have this issue with 0.4.6.8. This just started happening with 0.4.6.9.

[n8fr8]

Orbot isn't using 0.4.6.9, at least not my official RC build. We are still using .8

[syphyr]

I am using my own build of tor-android with latest Orbot tag. I just confirmed that this commit is causing the issue by reverting it.

[syphyr]

The way I reproduce the issue is start Orbot (select onion), then stop Orbot (select onion). Then Orbot gets stuck and I have to kill the process to start Orbot again.

[syphyr]

I also wanted to mention that this PR (guardianproject/orbot#561) does not fix this issue with Orbot