Răsfoiți Sursa

Fixed MobileLibrary/psi initialization error states

- Start failed to call ResetNoticeWriter on early error exits.

- StartSendFeedback left stopSendFeedback initialized in early exits cases,
  resulting in a potential nil pointer reference to sendFeedbackWaitGroup in
  StopSendFeedback.

- StartSendFeedback had a race condition now fixed with an "already started"
  check.

- Moved StartSendFeedback SetNoticeWriter call to after config initialization,
  matching Start and avoiding need for missing ResetNoticeWriter calls.
Rod Hynes 7 luni în urmă
părinte
comite
cec5b1869d
1 a modificat fișierele cu 32 adăugiri și 19 ștergeri
  1. 32 19
      MobileLibrary/psi/psi.go

+ 32 - 19
MobileLibrary/psi/psi.go

@@ -196,6 +196,7 @@ func Start(
 
 	err = psiphon.OpenDataStore(config)
 	if err != nil {
+		psiphon.ResetNoticeWriter()
 		return errors.Trace(err)
 	}
 
@@ -238,6 +239,7 @@ func Start(
 		stopController()
 		embeddedServerListWaitGroup.Wait()
 		psiphon.CloseDataStore()
+		psiphon.ResetNoticeWriter()
 		return errors.Trace(err)
 	}
 
@@ -361,7 +363,7 @@ var sendFeedbackMutex sync.Mutex
 var sendFeedbackCtx context.Context
 var stopSendFeedback context.CancelFunc
 var sendFeedbackWaitGroup *sync.WaitGroup
-var sendFeedbackSetNoticeWriter bool
+var sendFeedbackResetNoticeWriter bool
 
 // StartSendFeedback encrypts the provided diagnostics and then attempts to
 // upload the encrypted diagnostics to one of the feedback upload locations
@@ -418,22 +420,10 @@ func StartSendFeedback(
 	sendFeedbackMutex.Lock()
 	defer sendFeedbackMutex.Unlock()
 
-	sendFeedbackCtx, stopSendFeedback = context.WithCancel(context.Background())
-
-	// Unlike in Start, the provider is not wrapped in a newMutexPsiphonProvider
-	// or equivilent, as SendFeedback is not expected to be used in a memory
-	// constrained environment.
-
-	sendFeedbackSetNoticeWriter = noticeHandler != nil
-
-	if sendFeedbackSetNoticeWriter {
-		err := psiphon.SetNoticeWriter(psiphon.NewNoticeReceiver(
-			func(notice []byte) {
-				noticeHandler.Notice(string(notice))
-			}))
-		if err != nil {
-			return errors.Trace(err)
-		}
+	if stopSendFeedback != nil {
+		// Another goroutine invoked StartSendFeedback before the mutex lock
+		// was acquired.
+		return errors.TraceNew("already started")
 	}
 
 	config, err := psiphon.LoadConfig([]byte(configJson))
@@ -441,6 +431,10 @@ func StartSendFeedback(
 		return errors.Trace(err)
 	}
 
+	// Unlike in Start, the provider is not wrapped in a newMutexPsiphonProvider
+	// or equivalent, as SendFeedback is not expected to be used in a memory
+	// constrained environment.
+
 	// Set up callbacks.
 
 	config.NetworkConnectivityChecker = networkInfoProvider
@@ -469,6 +463,25 @@ func StartSendFeedback(
 		return errors.Trace(err)
 	}
 
+	setNoticeWriter := noticeHandler != nil
+
+	if setNoticeWriter {
+		err := psiphon.SetNoticeWriter(psiphon.NewNoticeReceiver(
+			func(notice []byte) {
+				noticeHandler.Notice(string(notice))
+			}))
+		if err != nil {
+			return errors.Trace(err)
+		}
+	}
+
+	// Initialize stopSendFeedback, which also serves as the "is started"
+	// flag, only after all early error returns.
+
+	sendFeedbackCtx, stopSendFeedback = context.WithCancel(context.Background())
+
+	sendFeedbackResetNoticeWriter = setNoticeWriter
+
 	sendFeedbackWaitGroup = new(sync.WaitGroup)
 	sendFeedbackWaitGroup.Add(1)
 	go func() {
@@ -496,11 +509,11 @@ func StopSendFeedback() {
 		sendFeedbackCtx = nil
 		stopSendFeedback = nil
 		sendFeedbackWaitGroup = nil
-		if sendFeedbackSetNoticeWriter {
+		if sendFeedbackResetNoticeWriter {
 			// Allow the notice handler to be garbage collected.
 			psiphon.ResetNoticeWriter()
 		}
-		sendFeedbackSetNoticeWriter = false
+		sendFeedbackResetNoticeWriter = false
 	}
 }