Merge pull request #687 from adam-p/clientlib-notices

clientlib fixes: StartTunnel re-entry, early stderr notices, clearing notice writer
Psiphon-Labs · Jun 19, 2024 · a963259 · a963259
2 parents 3b01244 + 6918f7a
commit a963259
Show file tree

Hide file tree

Showing 2 changed files with 251 additions and 140 deletions.
diff --git a/ClientLibrary/clientlib/clientlib.go b/ClientLibrary/clientlib/clientlib.go
@@ -140,63 +140,9 @@ func StartTunnel(
 	if !started.CompareAndSwap(false, true) {
 		return nil, errMultipleStart
 	}
-
-	config, err := psiphon.LoadConfig(configJSON)
-	if err != nil {
-		return nil, errors.TraceMsg(err, "failed to load config file")
-	}
-
-	// Use params.DataRootDirectory to set related config values.
-	if params.DataRootDirectory != nil {
-		config.DataRootDirectory = *params.DataRootDirectory
-
-		// Migrate old fields
-		config.MigrateDataStoreDirectory = *params.DataRootDirectory
-		config.MigrateObfuscatedServerListDownloadDirectory = *params.DataRootDirectory
-		config.MigrateRemoteServerListDownloadFilename = filepath.Join(*params.DataRootDirectory, "server_list_compressed")
-	}
-
-	if params.NetworkID != nil {
-		config.NetworkID = *params.NetworkID
-	}
-
-	if params.ClientPlatform != nil {
-		config.ClientPlatform = *params.ClientPlatform
-	} // else use the value in config
-
-	if params.EstablishTunnelTimeoutSeconds != nil {
-		config.EstablishTunnelTimeoutSeconds = params.EstablishTunnelTimeoutSeconds
-	} // else use the value in config
-
-	if config.UseNoticeFiles == nil && config.EmitDiagnosticNotices && params.EmitDiagnosticNoticesToFiles {
-		config.UseNoticeFiles = &psiphon.UseNoticeFiles{
-			RotatingFileSize:      0,
-			RotatingSyncFrequency: 0,
-		}
-	} // else use the value in the config
-
-	if params.DisableLocalSocksProxy != nil {
-		config.DisableLocalSocksProxy = *params.DisableLocalSocksProxy
-	} // else use the value in the config
-
-	if params.DisableLocalHTTPProxy != nil {
-		config.DisableLocalHTTPProxy = *params.DisableLocalHTTPProxy
-	} // else use the value in the config
-
-	// config.Commit must be called before calling config.SetParameters
-	// or attempting to connect.
-	err = config.Commit(true)
-	if err != nil {
-		return nil, errors.TraceMsg(err, "config.Commit failed")
-	}
-
-	// If supplied, apply the parameters delta
-	if len(paramsDelta) > 0 {
-		err = config.SetParameters("", false, paramsDelta)
-		if err != nil {
-			return nil, errors.TraceMsg(err, fmt.Sprintf("SetParameters failed for delta: %v", paramsDelta))
-		}
-	}
+	// There _must_ not be an early return between here and where tunnel.stop is deferred,
+	// otherwise `started` will not get set back to false and we will be unable to call
+	// StartTunnel again.
 
 	// Will be closed when the tunnel has successfully connected
 	connectedSignal := make(chan struct{})
@@ -206,7 +152,8 @@ func StartTunnel(
 	// Create the tunnel object
 	tunnel := new(PsiphonTunnel)
 
-	// Set up notice handling
+	// Set up notice handling. It is important to do this before config operations, as
+	// otherwise they will write notices to stderr.
 	psiphon.SetNoticeWriter(psiphon.NewNoticeReceiver(
 		func(notice []byte) {
 			var event NoticeEvent
@@ -247,11 +194,6 @@ func StartTunnel(
 			}
 		}))
 
-	err = psiphon.OpenDataStore(config)
-	if err != nil {
-		return nil, errors.TraceMsg(err, "failed to open data store")
-	}
-
 	// Create a cancelable context that will be used for stopping the tunnel
 	tunnelCtx, cancelTunnelCtx := context.WithCancel(ctx)
 
@@ -265,15 +207,82 @@ func StartTunnel(
 		cancelTunnelCtx()
 		tunnel.embeddedServerListWaitGroup.Wait()
 		tunnel.controllerWaitGroup.Wait()
+		// This is safe to call even if the data store hasn't been opened
 		psiphon.CloseDataStore()
 		started.Store(false)
+		// Clear our notice receiver, as it is no longer needed and we should let it be
+		// garbage-collected.
+		psiphon.SetNoticeWriter(io.Discard)
 	}
 
 	defer func() {
 		if retErr != nil {
 			tunnel.stop()
 		}
 	}()
+	// We have now set up our on-error cleanup and it is safe to have early error returns.
+
+	config, err := psiphon.LoadConfig(configJSON)
+	if err != nil {
+		return nil, errors.TraceMsg(err, "failed to load config file")
+	}
+
+	// Use params.DataRootDirectory to set related config values.
+	if params.DataRootDirectory != nil {
+		config.DataRootDirectory = *params.DataRootDirectory
+
+		// Migrate old fields
+		config.MigrateDataStoreDirectory = *params.DataRootDirectory
+		config.MigrateObfuscatedServerListDownloadDirectory = *params.DataRootDirectory
+		config.MigrateRemoteServerListDownloadFilename = filepath.Join(*params.DataRootDirectory, "server_list_compressed")
+	}
+
+	if params.NetworkID != nil {
+		config.NetworkID = *params.NetworkID
+	}
+
+	if params.ClientPlatform != nil {
+		config.ClientPlatform = *params.ClientPlatform
+	} // else use the value in config
+
+	if params.EstablishTunnelTimeoutSeconds != nil {
+		config.EstablishTunnelTimeoutSeconds = params.EstablishTunnelTimeoutSeconds
+	} // else use the value in config
+
+	if config.UseNoticeFiles == nil && config.EmitDiagnosticNotices && params.EmitDiagnosticNoticesToFiles {
+		config.UseNoticeFiles = &psiphon.UseNoticeFiles{
+			RotatingFileSize:      0,
+			RotatingSyncFrequency: 0,
+		}
+	} // else use the value in the config
+
+	if params.DisableLocalSocksProxy != nil {
+		config.DisableLocalSocksProxy = *params.DisableLocalSocksProxy
+	} // else use the value in the config
+
+	if params.DisableLocalHTTPProxy != nil {
+		config.DisableLocalHTTPProxy = *params.DisableLocalHTTPProxy
+	} // else use the value in the config
+
+	// config.Commit must be called before calling config.SetParameters
+	// or attempting to connect.
+	err = config.Commit(true)
+	if err != nil {
+		return nil, errors.TraceMsg(err, "config.Commit failed")
+	}
+
+	// If supplied, apply the parameters delta
+	if len(paramsDelta) > 0 {
+		err = config.SetParameters("", false, paramsDelta)
+		if err != nil {
+			return nil, errors.TraceMsg(err, fmt.Sprintf("SetParameters failed for delta: %v", paramsDelta))
+		}
+	}
+
+	err = psiphon.OpenDataStore(config)
+	if err != nil {
+		return nil, errors.TraceMsg(err, "failed to open data store")
+	}
 
 	// If specified, the embedded server list is loaded and stored. When there
 	// are no server candidates at all, we wait for this import to complete
@@ -374,10 +383,6 @@ func (tunnel *PsiphonTunnel) Stop() {
 	tunnel.stop()
 	tunnel.stop = nil
 	tunnel.controllerDial = nil
-
-	// Clear our notice receiver, as it is no longer needed and we should let it be
-	// garbage-collected.
-	psiphon.SetNoticeWriter(io.Discard)
 }
 
 // Dial connects to the specified address through the Psiphon tunnel.