Enable feedback and tunnel modes running concurrently in the same process

MobileLibrary/Android/PsiphonTunnel/PsiphonTunnel.java

@@ -305,13 +305,24 @@ public class PsiphonTunnel {
         // HostFeedbackHandler. The provided HostLogger will be called to log informational notices,
         // including warnings.
         //
-        // Warnings:
+        // If `startSendFeedback` is called concurrent with `start`:
+        //
+        // - logger MUST be null, otherwise start's notice handler and callbacks can be
+        //   hijacked.
+        //
+        // - configJson EmitDiagnosticNotices and UseNoticeFiles settings SHOULD be the same as
+        //   those passed to start, or else start's notice logging configuration can change.
+        //
+        // Additional warnings:
+        //
         // - Only one active upload is supported at a time. An ongoing upload will be cancelled if
         // this function is called again before it completes.
+        //
         // - An ongoing feedback upload started with startSendFeedback() should be stopped with
         // stopSendFeedback() before the process exits. This ensures that any underlying resources
         // are cleaned up; failing to do so may result in data store corruption or other undefined
         // behavior.
+        //
         // - PsiphonTunnel.startTunneling and startSendFeedback both make an attempt to migrate
         // persistent files from legacy locations in a one-time operation. If these functions are
         // called in parallel, then there is a chance that the migration attempts could execute at
@@ -328,6 +339,44 @@ public class PsiphonTunnel {
                         String psiphonConfig = buildPsiphonConfig(context, feedbackConfigJson,
                                 clientPlatformPrefix, clientPlatformSuffix, 0);
 
+                        PsiphonProviderNoticeHandler noticeHandler = null;
+                        if (logger != null) {
+                            noticeHandler = new PsiphonProviderNoticeHandler() {
+                                @Override
+                                public void notice(String noticeJSON) {
+
+                                    try {
+                                        JSONObject notice = new JSONObject(noticeJSON);
+
+                                        String noticeType = notice.getString("noticeType");
+
+                                        JSONObject data = notice.getJSONObject("data");
+
+                                        String diagnosticMessage = noticeType + ": " + data;
+                                        try {
+                                            callbackQueue.execute(new Runnable() {
+                                                @Override
+                                                public void run() {
+                                                    logger.onDiagnosticMessage(diagnosticMessage);
+                                                }
+                                            });
+                                        } catch (RejectedExecutionException ignored) {
+                                        }
+                                    } catch (java.lang.Exception e) {
+                                        try {
+                                            callbackQueue.execute(new Runnable() {
+                                                @Override
+                                                public void run() {
+                                                    logger.onDiagnosticMessage("Error handling notice " + e);
+                                                }
+                                            });
+                                        } catch (RejectedExecutionException ignored) {
+                                        }
+                                    }
+                                }
+                            };
+                        }
+
                         Psi.startSendFeedback(psiphonConfig, diagnosticsJson, uploadPath,
                                 new PsiphonProviderFeedbackHandler() {
                                     @Override
@@ -397,40 +446,7 @@ public class PsiphonTunnel {
                                         return PsiphonTunnel.hasIPv6Route(context, logger);
                                     }
                                 },
-                                new PsiphonProviderNoticeHandler() {
-                                    @Override
-                                    public void notice(String noticeJSON) {
-
-                                        try {
-                                            JSONObject notice = new JSONObject(noticeJSON);
-
-                                            String noticeType = notice.getString("noticeType");
-
-                                            JSONObject data = notice.getJSONObject("data");
-
-                                            String diagnosticMessage = noticeType + ": " + data;
-                                            try {
-                                                callbackQueue.execute(new Runnable() {
-                                                    @Override
-                                                    public void run() {
-                                                        logger.onDiagnosticMessage(diagnosticMessage);
-                                                    }
-                                                });
-                                            } catch (RejectedExecutionException ignored) {
-                                            }
-                                        } catch (java.lang.Exception e) {
-                                            try {
-                                                callbackQueue.execute(new Runnable() {
-                                                    @Override
-                                                    public void run() {
-                                                        logger.onDiagnosticMessage("Error handling notice " + e);
-                                                    }
-                                                });
-                                            } catch (RejectedExecutionException ignored) {
-                                            }
-                                        }
-                                    }
-                                },
+                                noticeHandler,
                                 false,   // Do not use IPv6 synthesizer for Android
                                 true     // Use hasIPv6Route on Android
                         );
@@ -589,7 +605,10 @@ public class PsiphonTunnel {
         try {
             hasRoute = hasIPv6Route(context);
         } catch (Exception e) {
-            logger.onDiagnosticMessage("failed to check IPv6 route: " + e.getMessage());
+            // logger may be null; see startSendFeedback.
+            if (logger != null) {
+                logger.onDiagnosticMessage("failed to check IPv6 route: " + e.getMessage());
+            }
         }
         // TODO: change to bool return value once gobind supports that type
         return hasRoute ? 1 : 0;

MobileLibrary/iOS/PsiphonTunnel/PsiphonTunnel/PsiphonTunnel.h

@@ -485,6 +485,10 @@ Returns the path where the rotated notices file will be created.
  reference; the caller is responsible for holding a strong reference.
  @param feedbackDelegate Delegate which `sendFeedbackCompleted(error)` is called on once when the operation completes; if
  error is non-nil, then the operation failed. Stored as a weak reference; the caller is responsible for holding a strong reference.
+ @warning if `startSendFeedback` is called concurrent with `start`:
+ (a) loggerDelegate MUST be nil, otherwise start's notice handler and callbacks can be hijacked;
+ (b) configJson EmitDiagnosticNotices and UseNoticeFiles settings SHOULD be the same as those passed to start,
+ or else start's notice logging configuration can change.
  @warning Only one active upload is supported at a time. An ongoing upload will be cancelled if this function is called again before it
  completes.
  @warning An ongoing feedback upload started with `startSendFeedback:` should be stopped with `stopSendFeedback` before the

MobileLibrary/iOS/PsiphonTunnel/PsiphonTunnel/PsiphonTunnel.m

@@ -1760,61 +1760,68 @@ typedef NS_ERROR_ENUM(PsiphonTunnelErrorDomain, PsiphonTunnelErrorCode) {
         PsiphonProviderFeedbackHandlerShim *innerFeedbackHandler =
             [[PsiphonProviderFeedbackHandlerShim alloc] initWithHandler:sendFeedbackCompleted];
 
-        // Convert notice to a diagnostic message and then log it.
-        void (^logNotice)(NSString * _Nonnull) = ^void(NSString * _Nonnull noticeJSON) {
-            __strong PsiphonTunnelFeedback *strongSelf = weakSelf;
-            if (strongSelf == nil) {
-                return;
-            }
-            __strong id<PsiphonTunnelLoggerDelegate> strongLogger = weakLogger;
-            if (strongLogger == nil) {
-                return;
-            }
-            if ([strongLogger respondsToSelector:@selector(onDiagnosticMessage:withTimestamp:)]) {
+        PsiphonProviderNoticeHandlerShim *noticeHandler = nil;
 
-                __block NSDictionary *notice = nil;
-                id block = ^(id obj, BOOL *ignored) {
-                    if (ignored == nil || *ignored == YES) {
-                        return;
-                    }
-                    notice = (NSDictionary *)obj;
-                };
-
-                id eh = ^(NSError *err) {
-                    notice = nil;
-                    logMessage([NSString stringWithFormat: @"Notice JSON parse failed: %@", err.description]);
-                };
+        if (loggerDelegate != nil) {
 
-                id parser = [SBJson4Parser parserWithBlock:block allowMultiRoot:NO unwrapRootArray:NO errorHandler:eh];
-                [parser parse:[noticeJSON dataUsingEncoding:NSUTF8StringEncoding]];
-
-                if (notice == nil) {
+            // Convert notice to a diagnostic message and then log it.
+            void (^logNotice)(NSString * _Nonnull) = ^void(NSString * _Nonnull noticeJSON) {
+                __strong PsiphonTunnelFeedback *strongSelf = weakSelf;
+                if (strongSelf == nil) {
                     return;
                 }
-
-                NSString *noticeType = notice[@"noticeType"];
-                if (noticeType == nil) {
-                    logMessage(@"Notice missing noticeType");
+                __strong id<PsiphonTunnelLoggerDelegate> strongLogger = weakLogger;
+                if (strongLogger == nil) {
                     return;
                 }
+                if ([strongLogger respondsToSelector:@selector(onDiagnosticMessage:withTimestamp:)]) {
 
-                NSDictionary *data = notice[@"data"];
-                if (data == nil) {
-                    return;
-                }
+                    __block NSDictionary *notice = nil;
+                    id block = ^(id obj, BOOL *ignored) {
+                        if (ignored == nil || *ignored == YES) {
+                            return;
+                        }
+                        notice = (NSDictionary *)obj;
+                    };
 
-                NSString *dataStr = [[[SBJson4Writer alloc] init] stringWithObject:data];
-                NSString *timestampStr = notice[@"timestamp"];
-                if (timestampStr == nil) {
-                    return;
+                    id eh = ^(NSError *err) {
+                        notice = nil;
+                        logMessage([NSString stringWithFormat: @"Notice JSON parse failed: %@", err.description]);
+                    };
+
+                    id parser = [SBJson4Parser parserWithBlock:block allowMultiRoot:NO unwrapRootArray:NO errorHandler:eh];
+                    [parser parse:[noticeJSON dataUsingEncoding:NSUTF8StringEncoding]];
+
+                    if (notice == nil) {
+                        return;
+                    }
+
+                    NSString *noticeType = notice[@"noticeType"];
+                    if (noticeType == nil) {
+                        logMessage(@"Notice missing noticeType");
+                        return;
+                    }
+
+                    NSDictionary *data = notice[@"data"];
+                    if (data == nil) {
+                        return;
+                    }
+
+                    NSString *dataStr = [[[SBJson4Writer alloc] init] stringWithObject:data];
+                    NSString *timestampStr = notice[@"timestamp"];
+                    if (timestampStr == nil) {
+                        return;
+                    }
+
+                    NSString *diagnosticMessage = [NSString stringWithFormat:@"%@: %@", noticeType, dataStr];
+                    dispatch_sync(strongSelf->callbackQueue, ^{
+                        [strongLogger onDiagnosticMessage:diagnosticMessage withTimestamp:timestampStr];
+                    });
                 }
+            };
 
-                NSString *diagnosticMessage = [NSString stringWithFormat:@"%@: %@", noticeType, dataStr];
-                dispatch_sync(strongSelf->callbackQueue, ^{
-                    [strongLogger onDiagnosticMessage:diagnosticMessage withTimestamp:timestampStr];
-                });
-            }
-        };
+            noticeHandler = [[PsiphonProviderNoticeHandlerShim alloc] initWithLogger:logNotice];
+        }
 
         NSDateFormatter *rfc3339Formatter = [PsiphonTunnel rfc3339Formatter];
 
@@ -1836,9 +1843,6 @@ typedef NS_ERROR_ENUM(PsiphonTunnelErrorDomain, PsiphonTunnelErrorCode) {
             }
         };
 
-        PsiphonProviderNoticeHandlerShim *noticeHandler =
-            [[PsiphonProviderNoticeHandlerShim alloc] initWithLogger:logNotice];
-
         PsiphonProviderNetwork *networkInfoProvider = [[PsiphonProviderNetwork alloc]
                                                        initWithTunnelWholeDevice:tunnelWholeDevice
                                                        logger:logger];

MobileLibrary/psi/psi.go

@@ -359,6 +359,7 @@ var sendFeedbackMutex sync.Mutex
 var sendFeedbackCtx context.Context
 var stopSendFeedback context.CancelFunc
 var sendFeedbackWaitGroup *sync.WaitGroup
+var sendFeedbackSetNoticeWriter bool
 
 // StartSendFeedback encrypts the provided diagnostics and then attempts to
 // upload the encrypted diagnostics to one of the feedback upload locations
@@ -372,17 +373,28 @@ var sendFeedbackWaitGroup *sync.WaitGroup
 // Only one active upload is supported at a time. An ongoing upload will be
 // cancelled if this function is called again before it completes.
 //
-// Warnings:
-//   - Should not be used with Start concurrently in the same process
+// If StartSendFeedback is called concurrent with Start:
+//
+//   - noticeHandler MUST be nil, otherwise Start's notice handler and
+//     callbacks can be hijacked.
+//
+//   - configJson EmitDiagnosticNotices and UseNoticeFiles settings SHOULD be
+//     the same as those passed to Start, or else Start's notice logging
+//     configuration can change.
+//
+// Additional warnings:
+//
 //   - An ongoing feedback upload started with StartSendFeedback should be
-//     stopped with StopSendFeedback before the process exists. This ensures that
+//     stopped with StopSendFeedback before the process exits. This ensures that
 //     any underlying resources are cleaned up; failing to do so may result in
 //     data store corruption or other undefined behavior.
+//
 //   - Start and StartSendFeedback both make an attempt to migrate persistent
 //     files from legacy locations in a one-time operation. If these functions
 //     are called in parallel, then there is a chance that the migration attempts
 //     could execute at the same time and result in non-fatal errors in one, or
 //     both, of the migration operations.
+//
 //   - Calling StartSendFeedback or StopSendFeedback on the same call stack
 //     that the PsiphonProviderFeedbackHandler.SendFeedbackCompleted() callback
 //     is delivered on can cause a deadlock. I.E. the callback code must return
@@ -410,10 +422,14 @@ func StartSendFeedback(
 	// or equivilent, as SendFeedback is not expected to be used in a memory
 	// constrained environment.
 
-	psiphon.SetNoticeWriter(psiphon.NewNoticeReceiver(
-		func(notice []byte) {
-			noticeHandler.Notice(string(notice))
-		}))
+	sendFeedbackSetNoticeWriter = noticeHandler != nil
+
+	if sendFeedbackSetNoticeWriter {
+		psiphon.SetNoticeWriter(psiphon.NewNoticeReceiver(
+			func(notice []byte) {
+				noticeHandler.Notice(string(notice))
+			}))
+	}
 
 	config, err := psiphon.LoadConfig([]byte(configJson))
 	if err != nil {
@@ -475,8 +491,11 @@ func StopSendFeedback() {
 		sendFeedbackCtx = nil
 		stopSendFeedback = nil
 		sendFeedbackWaitGroup = nil
-		// Allow the notice handler to be garbage collected.
-		psiphon.SetNoticeWriter(os.Stderr)
+		if sendFeedbackSetNoticeWriter {
+			// Allow the notice handler to be garbage collected.
+			psiphon.SetNoticeWriter(os.Stderr)
+		}
+		sendFeedbackSetNoticeWriter = false
 	}
 }