fix: StartTunnel re-entry, early stderr notices, clearing notice writer

Author: adam-p

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.

ClientLibrary/clientlib/clientlib_test.go

@@ -26,6 +26,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/Psiphon-Labs/psiphon-tunnel-core/psiphon"
 	"github.com/Psiphon-Labs/psiphon-tunnel-core/psiphon/common/protocol"
 )
 
@@ -418,3 +419,117 @@ func TestPsiphonTunnel_Dial(t *testing.T) {
 		})
 	}
 }
+
+// We had a problem where config-related notices were being printed to stderr before we
+// set the NoticeWriter. We want to make sure that no longer happens.
+func TestStartTunnelNoOutput(t *testing.T) {
+	configJSON, err := os.ReadFile("../../psiphon/controller_test.config")
+	if err != nil {
+		// What to do if config file is not present?
+		t.Skipf("error loading configuration file: %s", err)
+	}
+
+	var config map[string]interface{}
+	err = json.Unmarshal(configJSON, &config)
+	if err != nil {
+		t.Fatalf("json.Unmarshal failed: %v", err)
+	}
+
+	// Before starting the tunnel, set up a notice receiver. If it receives anything at
+	// all, that means that it would have been printed to stderr.
+	psiphon.SetNoticeWriter(psiphon.NewNoticeReceiver(
+		func(notice []byte) {
+			t.Fatalf("Received notice: %v", string(notice))
+		}))
+
+	// Use the legacy encoding to both exercise that case, and facilitate a
+	// gradual network upgrade to new encoding support.
+	config["TargetAPIEncoding"] = protocol.PSIPHON_API_ENCODING_JSON
+
+	configJSON, err = json.Marshal(config)
+	if err != nil {
+		t.Fatalf("json.Marshal failed: %v", err)
+	}
+
+	testDataDirName, err := os.MkdirTemp("", "psiphon-clientlib-test")
+	if err != nil {
+		t.Fatalf("ioutil.TempDir failed: %v", err)
+	}
+	defer os.RemoveAll(testDataDirName)
+
+	ctx := context.Background()
+
+	tunnel, err := StartTunnel(
+		ctx,
+		configJSON,
+		"",
+		Parameters{DataRootDirectory: &testDataDirName},
+		nil,
+		nil)
+
+	if err != nil {
+		t.Fatalf("StartTunnel() error = %v", err)
+	}
+	tunnel.Stop()
+}
+
+// We had a problem where a very early error could result in `started` being set to true
+// and not be set back to false, preventing StartTunnel from being re-callable.
+func TestStartTunnelReentry(t *testing.T) {
+	testDataDirName, err := os.MkdirTemp("", "psiphon-clientlib-test")
+	if err != nil {
+		t.Fatalf("ioutil.TempDir failed: %v", err)
+	}
+	defer os.RemoveAll(testDataDirName)
+
+	configJSON := []byte("BAD CONFIG JSON")
+
+	ctx := context.Background()
+
+	_, err = StartTunnel(
+		ctx,
+		configJSON,
+		"",
+		Parameters{DataRootDirectory: &testDataDirName},
+		nil,
+		nil)
+
+	if err == nil {
+		t.Fatalf("expected config error")
+	}
+
+	// Call again with a good config. Should work.
+	configJSON, err = os.ReadFile("../../psiphon/controller_test.config")
+	if err != nil {
+		// What to do if config file is not present?
+		t.Skipf("error loading configuration file: %s", err)
+	}
+
+	var config map[string]interface{}
+	err = json.Unmarshal(configJSON, &config)
+	if err != nil {
+		t.Fatalf("json.Unmarshal failed: %v", err)
+	}
+
+	// Use the legacy encoding to both exercise that case, and facilitate a
+	// gradual network upgrade to new encoding support.
+	config["TargetAPIEncoding"] = protocol.PSIPHON_API_ENCODING_JSON
+
+	configJSON, err = json.Marshal(config)
+	if err != nil {
+		t.Fatalf("json.Marshal failed: %v", err)
+	}
+
+	tunnel, err := StartTunnel(
+		ctx,
+		configJSON,
+		"",
+		Parameters{DataRootDirectory: &testDataDirName},
+		nil,
+		nil)
+
+	if err != nil {
+		t.Fatalf("StartTunnel() error = %v", err)
+	}
+	tunnel.Stop()
+}