From 69712e586bfe7d9fb17235476a08101e61a38fa1 Mon Sep 17 00:00:00 2001 From: Miro Date: Sun, 2 Jun 2024 12:06:58 -0400 Subject: [PATCH 1/4] Discovery fixes - Fix: shadowing nextUpdate causes NewTimer to fire instantly on each loop iteration - Fix: race conditions - Reduce TestDiscoveryTestClock runtime --- psiphon/server/discovery.go | 5 +++-- psiphon/server/discovery/discovery.go | 3 ++- psiphon/server/discovery/discovery_test.go | 4 ++-- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/psiphon/server/discovery.go b/psiphon/server/discovery.go index 96cdeb92e..b8e803f54 100644 --- a/psiphon/server/discovery.go +++ b/psiphon/server/discovery.go @@ -110,8 +110,6 @@ func (d *Discovery) reload(reloadedTactics bool) error { // Initialize and set underlying discovery component. Replaces old // component if discovery is already initialized. - oldDiscovery := d.discovery - discovery := discovery.MakeDiscovery( d.support.PsinetDatabase.GetDiscoveryServers(), discoveryStrategy) @@ -120,6 +118,7 @@ func (d *Discovery) reload(reloadedTactics bool) error { d.Lock() + oldDiscovery := d.discovery d.discovery = discovery d.currentStrategy = strategy @@ -143,6 +142,8 @@ func (d *Discovery) reload(reloadedTactics bool) error { // Stop stops discovery and cleans up underlying resources. func (d *Discovery) Stop() { + d.Lock() + defer d.Unlock() d.discovery.Stop() } diff --git a/psiphon/server/discovery/discovery.go b/psiphon/server/discovery/discovery.go index 608dc356b..28b705893 100644 --- a/psiphon/server/discovery/discovery.go +++ b/psiphon/server/discovery/discovery.go @@ -167,7 +167,8 @@ func (d *Discovery) Start() { // Note: servers with a discovery date range in the past are not // removed from d.all in case the wall clock has drifted; // otherwise, we risk removing them prematurely. - servers, nextUpdate := discoverableServers(d.all, d.clk) + var servers []*psinet.DiscoveryServer + servers, nextUpdate = discoverableServers(d.all, d.clk) // Update the set of discoverable servers. d.strategy.serversChanged(servers) diff --git a/psiphon/server/discovery/discovery_test.go b/psiphon/server/discovery/discovery_test.go index 3cb3bdb1a..2f293e7ef 100644 --- a/psiphon/server/discovery/discovery_test.go +++ b/psiphon/server/discovery/discovery_test.go @@ -149,9 +149,9 @@ func runDiscoveryTest(tt *discoveryTest, now time.Time) error { discovery.Start() for _, check := range tt.checks { - time.Sleep(1 * time.Second) // let async code complete + time.Sleep(10 * time.Millisecond) // let async code complete clk.SetNow(check.t) - time.Sleep(1 * time.Second) // let async code complete + time.Sleep(10 * time.Millisecond) // let async code complete discovered := discovery.SelectServers(net.IP{}) discoveredIPs := make([]string, len(discovered)) for i := range discovered { From 6d8f9a6414f07aa06972875bdcf029c687b5ff52 Mon Sep 17 00:00:00 2001 From: Eugene Fryntov Date: Thu, 6 Jun 2024 12:44:33 -0400 Subject: [PATCH 2/4] Fix notice handling for ConnectedServerRegion callback - Add new notice `NoticeConnectedServerRegion` to handle server region callbacks separately from `ActiveTunnel` - Update mobile libraries to handle `ConnectedServerRegion` notices --- MobileLibrary/Android/PsiphonTunnel/PsiphonTunnel.java | 2 +- .../iOS/PsiphonTunnel/PsiphonTunnel/PsiphonTunnel.m | 2 +- psiphon/controller.go | 2 ++ psiphon/notice.go | 7 +++++++ 4 files changed, 11 insertions(+), 2 deletions(-) diff --git a/MobileLibrary/Android/PsiphonTunnel/PsiphonTunnel.java b/MobileLibrary/Android/PsiphonTunnel/PsiphonTunnel.java index 9c4115d54..24913f2ac 100644 --- a/MobileLibrary/Android/PsiphonTunnel/PsiphonTunnel.java +++ b/MobileLibrary/Android/PsiphonTunnel/PsiphonTunnel.java @@ -1084,7 +1084,7 @@ private void handlePsiphonNotice(String noticeJSON) { enableUdpGwKeepalive(); } } - // Also report the tunnel's egress region to the host service + } else if (noticeType.equals("ConnectedServerRegion")) { mHostService.onConnectedServerRegion( notice.getJSONObject("data").getString("serverRegion")); } else if (noticeType.equals("ApplicationParameters")) { diff --git a/MobileLibrary/iOS/PsiphonTunnel/PsiphonTunnel/PsiphonTunnel.m b/MobileLibrary/iOS/PsiphonTunnel/PsiphonTunnel/PsiphonTunnel.m index bb756fb5b..f8c3b3c4e 100644 --- a/MobileLibrary/iOS/PsiphonTunnel/PsiphonTunnel/PsiphonTunnel.m +++ b/MobileLibrary/iOS/PsiphonTunnel/PsiphonTunnel/PsiphonTunnel.m @@ -1174,7 +1174,7 @@ - (void)handlePsiphonNotice:(NSString * _Nonnull)noticeJSON { }); } } - else if ([noticeType isEqualToString:@"ActiveTunnel"]) { + else if ([noticeType isEqualToString:@"ConnectedServerRegion"]) { id region = [notice valueForKeyPath:@"data.serverRegion"]; if (![region isKindOfClass:[NSString class]]) { [self logMessage:[NSString stringWithFormat: @"ActiveTunnel notice missing data.serverRegion: %@", noticeJSON]]; diff --git a/psiphon/controller.go b/psiphon/controller.go index f2217cc3d..8a9731cf2 100755 --- a/psiphon/controller.go +++ b/psiphon/controller.go @@ -1004,6 +1004,8 @@ loop: connectedTunnel.dialParams.ServerEntry.SupportsSSHAPIRequests(), connectedTunnel.dialParams.ServerEntry.Region) + NoticeConnectedServerRegion(connectedTunnel.dialParams.ServerEntry.Region) + if isFirstTunnel { // Signal a connected request on each 1st tunnel establishment. For diff --git a/psiphon/notice.go b/psiphon/notice.go index b41a3edef..ed856df21 100644 --- a/psiphon/notice.go +++ b/psiphon/notice.go @@ -687,6 +687,13 @@ func NoticeActiveTunnel(diagnosticID, protocol string, isTCS bool, serverRegion "serverRegion", serverRegion) } +// NoticeConnectedServerRegion reports the region of the connected server +func NoticeConnectedServerRegion(serverRegion string) { + singletonNoticeLogger.outputNotice( + "ConnectedServerRegion", 0, + "serverRegion", serverRegion) +} + // NoticeSocksProxyPortInUse is a failure to use the configured LocalSocksProxyPort func NoticeSocksProxyPortInUse(port int) { singletonNoticeLogger.outputNotice( From a2cb7dc445e6d1abd84c18b3118e7cddd313ef80 Mon Sep 17 00:00:00 2001 From: Eugene Fryntov Date: Thu, 6 Jun 2024 13:41:40 -0400 Subject: [PATCH 3/4] Add handling for non-diagnostic notices - Add a new flag `noticeIsNotDiagnostic` to indicate notices that should be emitted to the host application but not written to the diagnostic file - Update `outputNotice` function to handle the `noticeIsNotDiagnostic` flag - Update `NoticeBytesTransferred` to use the `noticeIsNotDiagnostic` flag --- psiphon/notice.go | 34 +++++++++++++++++++++------------- 1 file changed, 21 insertions(+), 13 deletions(-) diff --git a/psiphon/notice.go b/psiphon/notice.go index ed856df21..24cf5952c 100644 --- a/psiphon/notice.go +++ b/psiphon/notice.go @@ -198,11 +198,12 @@ func setNoticeFiles( } const ( - noticeIsDiagnostic = 1 - noticeIsHomepage = 2 - noticeClearHomepages = 4 - noticeSyncHomepages = 8 - noticeSkipRedaction = 16 + noticeIsDiagnostic = 1 + noticeIsHomepage = 2 + noticeClearHomepages = 4 + noticeSyncHomepages = 8 + noticeSkipRedaction = 16 + noticeIsNotDiagnostic = 32 ) // outputNotice encodes a notice in JSON and writes it to the output writer. @@ -279,17 +280,22 @@ func (nl *noticeLogger) outputNotice(noticeType string, noticeFlags uint32, args if nl.rotatingFile != nil { if !skipWriter { - skipWriter = (noticeFlags¬iceIsDiagnostic != 0) + // Skip writing to the host application if the notice is diagnostic + // and not explicitly marked as not diagnostic + skipWriter = (noticeFlags¬iceIsDiagnostic != 0) && (noticeFlags¬iceIsNotDiagnostic == 0) } if !skipRedaction { + // Only write to the rotating file if the notice is not explicitly marked as not diagnostic. + if noticeFlags¬iceIsNotDiagnostic == 0 { - err := nl.outputNoticeToRotatingFile(output) + err := nl.outputNoticeToRotatingFile(output) - if err != nil { - output := makeNoticeInternalError( - fmt.Sprintf("write rotating file failed: %s", err)) - nl.writer.Write(output) + if err != nil { + output := makeNoticeInternalError( + fmt.Sprintf("write rotating file failed: %s", err)) + nl.writer.Write(output) + } } } } @@ -839,10 +845,12 @@ func NoticeClientUpgradeDownloaded(filename string) { // transferred since the last NoticeBytesTransferred. This is not a diagnostic // notice: the user app has requested this notice with EmitBytesTransferred // for functionality such as traffic display; and this frequent notice is not -// intended to be included with feedback. +// intended to be included with feedback. The noticeIsNotDiagnostic flag +// ensures that these notices are emitted to the host application but not written +// to the diagnostic file to avoid cluttering it with frequent updates. func NoticeBytesTransferred(diagnosticID string, sent, received int64) { singletonNoticeLogger.outputNotice( - "BytesTransferred", 0, + "BytesTransferred", noticeIsNotDiagnostic, "diagnosticID", diagnosticID, "sent", sent, "received", received) From 50dfa0287373273ee75eae8ebbd153b458f812ef Mon Sep 17 00:00:00 2001 From: Eugene Fryntov Date: Thu, 6 Jun 2024 15:25:43 -0400 Subject: [PATCH 4/4] Remove serverRegion paramater from NoticeActiveTunnel The parameter will be reported by `NoticeConnectedServerRegion` --- psiphon/controller.go | 3 +-- psiphon/notice.go | 5 ++--- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/psiphon/controller.go b/psiphon/controller.go index 8a9731cf2..c84ee5bc0 100755 --- a/psiphon/controller.go +++ b/psiphon/controller.go @@ -1001,8 +1001,7 @@ loop: NoticeActiveTunnel( connectedTunnel.dialParams.ServerEntry.GetDiagnosticID(), connectedTunnel.dialParams.TunnelProtocol, - connectedTunnel.dialParams.ServerEntry.SupportsSSHAPIRequests(), - connectedTunnel.dialParams.ServerEntry.Region) + connectedTunnel.dialParams.ServerEntry.SupportsSSHAPIRequests()) NoticeConnectedServerRegion(connectedTunnel.dialParams.ServerEntry.Region) diff --git a/psiphon/notice.go b/psiphon/notice.go index 24cf5952c..bcf734c41 100644 --- a/psiphon/notice.go +++ b/psiphon/notice.go @@ -684,13 +684,12 @@ func NoticeRequestedTactics(dialParams *DialParameters) { } // NoticeActiveTunnel is a successful connection that is used as an active tunnel for port forwarding -func NoticeActiveTunnel(diagnosticID, protocol string, isTCS bool, serverRegion string) { +func NoticeActiveTunnel(diagnosticID, protocol string, isTCS bool) { singletonNoticeLogger.outputNotice( "ActiveTunnel", noticeIsDiagnostic, "diagnosticID", diagnosticID, "protocol", protocol, - "isTCS", isTCS, - "serverRegion", serverRegion) + "isTCS", isTCS) } // NoticeConnectedServerRegion reports the region of the connected server