Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow TorService to be used in a separate process #61

Closed
wants to merge 2 commits into from
Closed

Allow TorService to be used in a separate process #61

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b1a27f1
Expose FileDescriptor of control socket to enable running TorService …
grote Dec 9, 2021
7314cf5
Use __OwningControllerProcess option to terminate Tor when the owning…
grote Dec 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion tor-android-binary/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ android {
dependencies {
implementation fileTree(dir: 'libs', include: ['*.so'])
api 'androidx.localbroadcastmanager:localbroadcastmanager:1.0.0'
api 'info.guardianproject:jtorctl:0.4.2.5'
api 'info.guardianproject:jtorctl:0.4.5.7'
eighthave marked this conversation as resolved.
Show resolved Hide resolved

androidTestImplementation 'androidx.test:core:1.4.0'
androidTestImplementation 'androidx.test:runner:1.4.0'
Expand Down
20 changes: 19 additions & 1 deletion tor-android-binary/src/main/java/org/torproject/jni/TorService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,12 @@ public class TorService extends Service {
*/
private static final String ACTION_STOP = "org.torproject.android.intent.action.STOP";

/**
* A integer intent extra containing the PID of the owning process,
* if this service is run in a dedicated process.
*/
public static final String EXTRA_PID = "org.torproject.android.intent.extra.PID";

/**
* {@link Intent} sent by this app with {@code ON/OFF/STARTING/STOPPING} status
* included as an {@link #EXTRA_STATUS} {@code String}. Your app should
Expand Down Expand Up @@ -115,6 +121,10 @@ public static File getDefaultsTorrc(Context context) {
return new File(getAppTorServiceDir(context), "torrc-defaults");
}

public static FileDescriptor getControlSocketFileDescriptor(Context context) {
return prepareFileDescriptor(getControlSocket(context).getAbsolutePath());
}

private static File getControlSocket(Context context) {
if (controlSocket == null) {
controlSocket = new File(getAppTorServiceDataDir(context), CONTROL_SOCKET_NAME);
Expand Down Expand Up @@ -167,6 +177,8 @@ private static File getAppTorServiceDataDir(Context context) {
private long torConfiguration = -1;
private int torControlFd = -1;

private int owningProcessPid = -1;

private TorControlConnection torControlConnection;

/**
Expand Down Expand Up @@ -200,6 +212,7 @@ public TorService getService() {

@Override
public IBinder onBind(Intent intent) {
owningProcessPid = intent.getIntExtra(EXTRA_PID, -1);
return binder;
}

Expand Down Expand Up @@ -264,7 +277,7 @@ public void onEvent(int event, @Nullable String name) {
throw new IllegalStateException("cannot read " + controlSocket);
}

FileDescriptor controlSocketFd = prepareFileDescriptor(getControlSocket(TorService.this).getAbsolutePath());
FileDescriptor controlSocketFd = getControlSocketFileDescriptor(TorService.this);
InputStream is = new FileInputStream(controlSocketFd);
OutputStream os = new FileOutputStream(controlSocketFd);
torControlConnection = new TorControlConnection(is, os);
Expand Down Expand Up @@ -346,6 +359,10 @@ public void run() {
"--LogMessageDomains", "1",
"--TruncateLogFile", "1"
));
if (owningProcessPid != -1) {
lines.add("--__OwningControllerProcess");
lines.add(Integer.toString(owningProcessPid));
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to ensure that Tor doesn't outlive the app (for more than 15sec) if it crashes. Otherwise, it would continue idling on the system (potentially indefinitely) consuming resources.

Copy link
Member

@eighthave eighthave Dec 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Android has native mechanisms for ensuring that a Service does not outlive its app. Binding is one of them. TorService works much more in the Android space than tor daemon ever did. This kind of thing seems like a holdover from running tor as a daemon. (If we eliminate the control port socket, then it will be even more in the direction of native Android.)

Copy link
Member

@eighthave eighthave Dec 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read up a bit on this option, it doesn't seem like something that should be built into TorService because it is about the daemon-style lifecycle and the control port. If it is something you want to use, you can generate your own torrc and TorService will use that file.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use this option in Briar to prevent the Tor process from outliving the controller process, which was a recurrent source of bugs for both Briar and Orbot in the past. When owingProcessPid != -1 the controller and Tor library are running in different processes, so that possibility resurfaces.

As far as I know, Android doesn't guarantee that a service will be destroyed if a bound client in another process crashes. The service may be cached, for example. So I don't think it's wise to rely on Android destroying the service.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another quick thought here: when TorService is running as a foreground service (which we're doing in OnionShare because Android gives various power management advantages to foreground services), we've found than simply unbinding is not enough to stop or destroy the service. I don't know whether the same applies when a bound client dies, rather than unbinding, but I think it would at least be worth checking that the service gets destroyed before we stop using __OwningControllerProcess.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we'll want to use WorkManager for OnionShare, as the tasks started by the app are expected to start immediately, not to be scheduled for later execution when some constraints are met.

Those other tasks should be in a Service instance that is separate from TorService. OnionShare could have multiple uploads running at the same time. Each of those would run in an instance of UploadService that is bound to OnionShare's unique TorService. Or there could be a CoordinatorService which manages the notifications and is set as a foreground service.

That's one possible way of structuring things, yes. But it would be nice if the library didn't impose a specific architecture on the application.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WorkManager now considers foreground processing a first class use case: https://www.droidcon.com/2021/11/17/workmanager-back-to-the-foreground/ see at 6:30. WorkManager is clearly the way forward with Tor on Android.

That's one possible way of structuring things, yes. But it would be nice if the library didn't impose a specific architecture on the application.

This library does not impose that kind of architecture, Android does. A central goal of this library is to make Tor behave natively on Android. Android works quite differently than UNIX, Windows, and macOS. Using desktop lifecycle constructs on Android and iOS works quite poorly, that's why this is a goal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you refer to Expedited work then that is "for tasks that are important to the user and which complete within a few minutes." This not the case for the OnionShare use-case where you might want to leave the share running for more than a day without the fear of yet another system service killing you early.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See also the docs on quotas: https://developer.android.com/topic/libraries/architecture/workmanager/how-to/define-work#quotas

If you want a library to be used, you really shouldn't impose a specific architecture on the application. If you want an easy way to use this with WorkManager, it is fine to add this as an extra wrapper around the general solution.

IMHO even offering only a Service and broadcasts is already going to far into a certain direction.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the link you sent:

While your app is in the foreground, quotas won’t limit the execution of expedited work.

Here's more on using WorkManager for long running services, "Example use-cases for this new feature include bulk uploads or downloads":
https://developer.android.com/topic/libraries/architecture/workmanager/advanced/long-running

(github messed up the threading so this is a repost)

String[] verifyLines = lines.toArray(new String[0]);
if (!mainConfigurationSetCommandLine(verifyLines)) {
throw new IllegalArgumentException("Setting command line failed: " + Arrays.toString(verifyLines));
Expand Down Expand Up @@ -377,6 +394,7 @@ public void run() {
mainConfigurationFree();
Log.i(TAG, "Releasing lock");
runLock.unlock();
if (owningProcessPid != -1) stopSelf();
eighthave marked this conversation as resolved.
Show resolved Hide resolved
}

} catch (IllegalStateException | IllegalArgumentException | InterruptedException e) {
Expand Down