From b10eea47e7ea1d66b7e06c1894591d540900fbc8 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Mon, 12 Sep 2022 07:33:34 +0200 Subject: [PATCH] feat(webconnectivity@v0.5): flag case where noone resolved any address (#953) See https://github.com/ooni/probe/issues/2290 While there, notice that in such a case the priority selector would hang because of the WaitGroup, so get rid of the WaitGroup and accept that the priority selector is going to hang around for the whole duration of the measurement in some cases. The cancellable `measurer.go`'s context will cause the priority selector to eventually exit when we return from `measurer.go`'s `Run` method. --- .../webconnectivity/analysiscore.go | 63 ++++++++++++++++++- .../webconnectivity/dnsresolvers.go | 2 +- .../experiment/webconnectivity/measurer.go | 2 +- .../experiment/webconnectivity/priority.go | 10 +-- .../experiment/webconnectivity/testkeys.go | 6 ++ 5 files changed, 72 insertions(+), 11 deletions(-) diff --git a/internal/experiment/webconnectivity/analysiscore.go b/internal/experiment/webconnectivity/analysiscore.go index 1ed20bb..e92c908 100644 --- a/internal/experiment/webconnectivity/analysiscore.go +++ b/internal/experiment/webconnectivity/analysiscore.go @@ -67,7 +67,7 @@ const ( // values of .Blocking and .Accessible: // // +--------------------------------------+----------------+-------------+ -// | XBlockingFlags | .Blocking | .Accessible | +// | .BlockingFlags | .Blocking | .Accessible | // +--------------------------------------+----------------+-------------+ // | (& DNSBlocking) != 0 | "dns" | false | // +--------------------------------------+----------------+-------------+ @@ -135,6 +135,15 @@ func (tk *TestKeys) analysisToplevel(logger model.Logger) { ) default: + if tk.analysisNullNullDetectNoAddrs(logger) { + tk.Blocking = false + tk.Accessible = false + logger.Infof( + "NO_AVAILABLE_ADDRS: flags=%d, accessible=%+v, blocking=%+v", + tk.BlockingFlags, tk.Accessible, tk.Blocking, + ) + return + } tk.Blocking = nil tk.Accessible = nil logger.Warnf( @@ -143,3 +152,55 @@ func (tk *TestKeys) analysisToplevel(logger model.Logger) { ) } } + +const ( + // analysisFlagNullNullNoAddrs indicates neither the probe nor the TH were + // able to get any IP addresses from any resolver. + analysisFlagNullNullNoAddrs = 1 << iota +) + +// analysisNullNullDetectNoAddrs attempts to see whether we +// ended up into the .Blocking = nil, .Accessible = nil case because +// the domain is expired and all queries returned no addresses. +// +// See https://github.com/ooni/probe/issues/2290 for further +// documentation about the issue we're solving here. +// +// It would be tempting to check specifically for NXDOMAIN here, but we +// know it is problematic do that. In fact, on Android the getaddrinfo +// resolver always returns EAI_NODATA on error, regardless of the actual +// error that may have occurred in the Android DNS backend. +// +// See https://github.com/ooni/probe/issues/2029 for more information +// on Android's getaddrinfo behavior. +func (tk *TestKeys) analysisNullNullDetectNoAddrs(logger model.Logger) bool { + if tk.Control == nil { + // we need control data to say we're in this case + return false + } + for _, query := range tk.Queries { + if len(query.Answers) > 0 { + // when a query has answers, we're not in the NoAddresses case + return false + } + } + if len(tk.TCPConnect) > 0 { + // if we attempted TCP connect, we're not in the NoAddresses case + return false + } + if len(tk.TLSHandshakes) > 0 { + // if we attempted TLS handshakes, we're not in the NoAddresses case + return false + } + if len(tk.Control.DNS.Addrs) > 0 { + // when the TH resolved addresses, we're not in the NoAddresses case + return false + } + if len(tk.Control.TCPConnect) > 0 { + // when the TH used addresses, we're not in the NoAddresses case + return false + } + logger.Infof("Neither the probe nor the TH resolved any addresses") + tk.NullNullFlags |= analysisFlagNullNullNoAddrs + return true +} diff --git a/internal/experiment/webconnectivity/dnsresolvers.go b/internal/experiment/webconnectivity/dnsresolvers.go index 856b4fa..89465f1 100644 --- a/internal/experiment/webconnectivity/dnsresolvers.go +++ b/internal/experiment/webconnectivity/dnsresolvers.go @@ -172,7 +172,7 @@ func (t *DNSResolvers) Run(parentCtx context.Context) { } // create priority selector - ps := newPrioritySelector(parentCtx, t.ZeroTime, t.TestKeys, t.Logger, t.WaitGroup, addresses) + ps := newPrioritySelector(parentCtx, t.ZeroTime, t.TestKeys, t.Logger, addresses) // fan out a number of child async tasks to use the IP addrs t.startCleartextFlows(parentCtx, ps, addresses) diff --git a/internal/experiment/webconnectivity/measurer.go b/internal/experiment/webconnectivity/measurer.go index 21ce49d..592c8ba 100644 --- a/internal/experiment/webconnectivity/measurer.go +++ b/internal/experiment/webconnectivity/measurer.go @@ -36,7 +36,7 @@ func (m *Measurer) ExperimentName() string { // ExperimentVersion implements model.ExperimentMeasurer. func (m *Measurer) ExperimentVersion() string { - return "0.5.10" + return "0.5.11" } // Run implements model.ExperimentMeasurer. diff --git a/internal/experiment/webconnectivity/priority.go b/internal/experiment/webconnectivity/priority.go index 4f84af1..6c043dd 100644 --- a/internal/experiment/webconnectivity/priority.go +++ b/internal/experiment/webconnectivity/priority.go @@ -30,7 +30,6 @@ import ( "context" "fmt" "net" - "sync" "time" "github.com/ooni/probe-cli/v3/internal/model" @@ -80,7 +79,6 @@ func newPrioritySelector( zeroTime time.Time, tk *TestKeys, logger model.Logger, - wg *sync.WaitGroup, addrs []DNSEntry, ) *prioritySelector { ps := &prioritySelector{ @@ -107,8 +105,7 @@ func newPrioritySelector( ps.nhttps++ } } - wg.Add(1) - go ps.selector(ctx, wg) + go ps.selector(ctx) return ps } @@ -151,10 +148,7 @@ func (ps *prioritySelector) permissionToFetch(address string) bool { // background goroutine and terminates when [ctx] is done. // // This function implements https://github.com/ooni/probe/issues/2276. -func (ps *prioritySelector) selector(ctx context.Context, wg *sync.WaitGroup) { - // synchronize with the parent - defer wg.Done() - +func (ps *prioritySelector) selector(ctx context.Context) { // Implementation note: setting an arbitrary timeout here would // be ~an issue because we want this goroutine to be available in // case the only connections from which we could fetch a webpage diff --git a/internal/experiment/webconnectivity/testkeys.go b/internal/experiment/webconnectivity/testkeys.go index 94d6282..327bb7c 100644 --- a/internal/experiment/webconnectivity/testkeys.go +++ b/internal/experiment/webconnectivity/testkeys.go @@ -90,6 +90,11 @@ type TestKeys struct { // BlockingFlags contains blocking flags. BlockingFlags int64 `json:"x_blocking_flags"` + // NullNullFlags explains why we determined that a measurement is not + // failed by detecting specific conditions that would have otherwise + // caused .Accessible = nil and .Blocking = nil + NullNullFlags int64 `json:"x_null_null_flags"` + // BodyLength match tells us whether the body length matches. BodyLengthMatch *bool `json:"body_length_match"` @@ -334,6 +339,7 @@ func NewTestKeys() *TestKeys { DNSConsistency: "", HTTPExperimentFailure: nil, BlockingFlags: 0, + NullNullFlags: 0, BodyLengthMatch: nil, HeadersMatch: nil, StatusCodeMatch: nil,