From b5da8be1839d8692b9c27adfefa197401dda8a74 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 12 Jan 2022 14:26:10 +0100 Subject: [PATCH] fix(netxlite): robust {ReadAll,Copy}Context with wrapped io.EOF (#661) * chore(netxlite): add currently failing test case This diff introduces a test cases that will fail because of the reason explained in https://github.com/ooni/probe/issues/1965. * chore(netxlite/iox_test.go): add failing unit tests These tests directly show how the Go implementation of ReadAll and Copy has the issue of checking for io.EOF equality. * fix(netxlite): make {ReadAll,Copy}Context robust to wrapped io.EOF The fix is simple: we just need to check for `errors.Is(err, io.EOF)` after either io.ReadAll or io.Copy has returned. When this condition is true, we need to convert the error back to `nil` as it ought to be. While there, observe that the unit tests I committed in the previous commit are wrongly asserting that the error must be wrapped. This assertion is not correct, because in both cases we have just ensured that the returned error is `nil` (i.e., success). See https://github.com/ooni/probe/issues/1965. * cleanup: remove previous workaround for wrapped io.EOF These workarounds were partial, meaning that they would cover some cases in which the issue occurred but not all of them. Handling the problem in `netxlite.{ReadAll,Copy}Context` is the right thing to do _as long as_ we always use these functions instead of `io.{ReadAll,Copy}`. This is why it's now important to ensure we clearly mention that inside of the `CONTRIBUTING.md` guide and to also ensure that we're not using these functions in the code base. * fix(urlgetter): repair tests who assumed to see EOF error Now that we have established that we should normalize EOF when reading bodies like the stdlib does and now that it's clear why our behavior diverged from the stdlib, we also need to repair all the tests that assumed this incorrect behavior. * fix(all): don't use io{,util}.{Copy,ReadAll} * feat: add checks to ensure we don't use io.{Copy,ReadAll} * doc(netxlite): document we know how to deal w/ wrapped io.EOF * fix(nocopyreadall.bash): add exception for i/n/iox.go --- .github/workflows/checks.yml | 16 +++++ .../experiment/urlgetter/runner_test.go | 13 ++-- internal/engine/experiment/websteps/http.go | 5 +- internal/engine/netx/httptransport/saver.go | 18 ------ .../netx/httptransport/saver_internal_test.go | 51 --------------- internal/measurex/http.go | 3 - internal/netxlite/filtering/tls.go | 6 +- internal/netxlite/integration_test.go | 31 ++++++++++ internal/netxlite/iox.go | 17 +++++ internal/netxlite/iox_test.go | 62 +++++++++++++++++++ script/nocopyreadall.bash | 27 ++++++++ 11 files changed, 166 insertions(+), 83 deletions(-) create mode 100644 .github/workflows/checks.yml delete mode 100644 internal/engine/netx/httptransport/saver_internal_test.go create mode 100755 script/nocopyreadall.bash diff --git a/.github/workflows/checks.yml b/.github/workflows/checks.yml new file mode 100644 index 0000000..17ab01d --- /dev/null +++ b/.github/workflows/checks.yml @@ -0,0 +1,16 @@ +# checks performs several code quality checks +name: checks +on: + pull_request: + push: + branches: + - "master" +jobs: + test: + runs-on: "${{ matrix.os }}" + strategy: + matrix: + os: [ "ubuntu-20.04" ] + steps: + - uses: actions/checkout@v2 + - run: ./script/nocopyreadall.bash diff --git a/internal/engine/experiment/urlgetter/runner_test.go b/internal/engine/experiment/urlgetter/runner_test.go index 9f9947b..81f1dcb 100644 --- a/internal/engine/experiment/urlgetter/runner_test.go +++ b/internal/engine/experiment/urlgetter/runner_test.go @@ -3,7 +3,6 @@ package urlgetter_test import ( "context" "errors" - "io" "net/http" "net/http/httptest" "strings" @@ -162,7 +161,7 @@ func TestRunnerHTTPNoRedirect(t *testing.T) { } } -func TestRunnerHTTPCannotReadBody(t *testing.T) { +func TestRunnerHTTPWithConnectionClosedByServer(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { hijacker, ok := w.(http.Hijacker) if !ok { @@ -183,8 +182,8 @@ func TestRunnerHTTPCannotReadBody(t *testing.T) { Target: server.URL, } err := r.Run(context.Background()) - if !errors.Is(err, io.EOF) { - t.Fatal("not the error we expected") + if err != nil { + t.Fatal(err) } } @@ -206,7 +205,7 @@ func TestRunnerHTTPWeHandle400Correctly(t *testing.T) { } } -func TestRunnerHTTPCannotReadBodyWinsOver400(t *testing.T) { +func TestRunnerHTTPWithConnectionClosedByServerAnd400(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { hijacker, ok := w.(http.Hijacker) if !ok { @@ -228,8 +227,8 @@ func TestRunnerHTTPCannotReadBodyWinsOver400(t *testing.T) { Target: server.URL, } err := r.Run(context.Background()) - if !errors.Is(err, io.EOF) { - t.Fatal("not the error we expected") + if !errors.Is(err, urlgetter.ErrHTTPRequestFailed) { + t.Fatal("not the error we expected", err) } } diff --git a/internal/engine/experiment/websteps/http.go b/internal/engine/experiment/websteps/http.go index ec6a965..3b1ad34 100644 --- a/internal/engine/experiment/websteps/http.go +++ b/internal/engine/experiment/websteps/http.go @@ -1,8 +1,9 @@ package websteps import ( - "io" "net/http" + + "github.com/ooni/probe-cli/v3/internal/netxlite" ) // HTTPDo performs the HTTP check. @@ -25,7 +26,7 @@ func HTTPDo(req *http.Request, transport http.RoundTripper) (*http.Response, []b return nil, nil, err } defer resp.Body.Close() - body, err := io.ReadAll(resp.Body) + body, err := netxlite.ReadAllContext(req.Context(), resp.Body) if err != nil { return resp, nil, nil } diff --git a/internal/engine/netx/httptransport/saver.go b/internal/engine/netx/httptransport/saver.go index 3ee3d93..ba179e0 100644 --- a/internal/engine/netx/httptransport/saver.go +++ b/internal/engine/netx/httptransport/saver.go @@ -3,7 +3,6 @@ package httptransport import ( "bytes" "context" - "errors" "io" "net/http" "net/http/httptrace" @@ -141,7 +140,6 @@ func (txp SaverBodyHTTPTransport) RoundTrip(req *http.Request) (*http.Response, return nil, err } data, err := saverSnapRead(req.Context(), resp.Body, snapsize) - err = ignoreExpectedEOF(err, resp) if err != nil { resp.Body.Close() return nil, err @@ -156,22 +154,6 @@ func (txp SaverBodyHTTPTransport) RoundTrip(req *http.Request) (*http.Response, return resp, nil } -// ignoreExpectedEOF converts an error signalling the end of the body -// into a success. We know that we are in such condition when the -// resp.Close hint flag is set to true. (Thanks, stdlib!) -// -// See https://github.com/ooni/probe-engine/issues/1191 for an analysis -// of how this error was impacting measurements and data quality. -func ignoreExpectedEOF(err error, resp *http.Response) error { - if err == nil { - return nil - } - if errors.Is(err, io.EOF) && resp.Close { - return nil - } - return err -} - func saverSnapRead(ctx context.Context, r io.ReadCloser, snapsize int) ([]byte, error) { return netxlite.ReadAllContext(ctx, io.LimitReader(r, int64(snapsize))) } diff --git a/internal/engine/netx/httptransport/saver_internal_test.go b/internal/engine/netx/httptransport/saver_internal_test.go deleted file mode 100644 index 5777d49..0000000 --- a/internal/engine/netx/httptransport/saver_internal_test.go +++ /dev/null @@ -1,51 +0,0 @@ -package httptransport - -import ( - "errors" - "fmt" - "io" - "net/http" - "testing" -) - -func composeWithEOFError(msg string) error { - return fmt.Errorf("%w: %s", io.EOF, msg) -} - -func TestIgnoreExpectedEOFWithNoError(t *testing.T) { - if err := ignoreExpectedEOF(nil, nil); err != nil { - t.Fatal(err) - } -} - -func TestIgnoreExpectedEOFWithEOFErrorButNoCloseHint(t *testing.T) { - resp := &http.Response{} - in := composeWithEOFError("antani") - if err := ignoreExpectedEOF(in, resp); !errors.Is(err, io.EOF) { - t.Fatalf("not the error we expected: %+v", err) - } -} - -func TestIgnoreExpectedEOFWithEOFErrorAndCloseHint(t *testing.T) { - resp := &http.Response{Close: true} - in := composeWithEOFError("antani") - if err := ignoreExpectedEOF(in, resp); err != nil { - t.Fatal(err) - } -} - -func TestIgnoreExpectedEOFAnyOtherErrorAndCloseHint(t *testing.T) { - resp := &http.Response{Close: true} - in := errors.New("antani") - if err := ignoreExpectedEOF(in, resp); !errors.Is(err, in) { - t.Fatalf("not the error we expected: %+v", err) - } -} - -func TestIgnoreExpectedEOFAnyOtherErrorAndNoCloseHint(t *testing.T) { - resp := &http.Response{Close: false /*explicit*/} - in := errors.New("antani") - if err := ignoreExpectedEOF(in, resp); !errors.Is(err, in) { - t.Fatalf("not the error we expected: %+v", err) - } -} diff --git a/internal/measurex/http.go b/internal/measurex/http.go index 7a93b51..5222533 100644 --- a/internal/measurex/http.go +++ b/internal/measurex/http.go @@ -188,9 +188,6 @@ func (txp *HTTPTransportDB) RoundTrip(req *http.Request) (*http.Response, error) rt.ResponseHeaders = resp.Header r := io.LimitReader(resp.Body, txp.MaxBodySnapshotSize) body, err := netxlite.ReadAllContext(req.Context(), r) - if errors.Is(err, io.EOF) && resp.Close { - err = nil // we expected to see an EOF here, so no real error - } if err != nil { rt.Finished = time.Since(txp.Begin).Seconds() rt.Failure = NewFailure(err) diff --git a/internal/netxlite/filtering/tls.go b/internal/netxlite/filtering/tls.go index 0810c85..32b092e 100644 --- a/internal/netxlite/filtering/tls.go +++ b/internal/netxlite/filtering/tls.go @@ -1,12 +1,14 @@ package filtering import ( + "context" "crypto/tls" "errors" - "io" "net" "strings" "sync" + + "github.com/ooni/probe-cli/v3/internal/netxlite" ) // TLSAction is a TLS filtering action that this proxy should take. @@ -235,5 +237,5 @@ func (p *TLSProxy) connectingToMyself(conn net.Conn) bool { // forward will forward the traffic. func (p *TLSProxy) forward(wg *sync.WaitGroup, left net.Conn, right net.Conn) { defer wg.Done() - io.Copy(left, right) + netxlite.CopyContext(context.Background(), left, right) } diff --git a/internal/netxlite/integration_test.go b/internal/netxlite/integration_test.go index 937e7c7..66115eb 100644 --- a/internal/netxlite/integration_test.go +++ b/internal/netxlite/integration_test.go @@ -6,6 +6,7 @@ import ( "fmt" "net" "net/http" + "net/http/httptest" "net/url" "testing" "time" @@ -16,6 +17,7 @@ import ( "github.com/ooni/probe-cli/v3/internal/netxlite" "github.com/ooni/probe-cli/v3/internal/netxlite/filtering" "github.com/ooni/probe-cli/v3/internal/netxlite/quictesting" + "github.com/ooni/probe-cli/v3/internal/runtimex" utls "gitlab.com/yawning/utls.git" ) @@ -490,6 +492,35 @@ func TestHTTPTransport(t *testing.T) { resp.Body.Close() client.CloseIdleConnections() }) + + t.Run("we can read the body when the connection is closed", func(t *testing.T) { + // See https://github.com/ooni/probe/issues/1965 + srvr := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + hj := w.(http.Hijacker) // panic if not possible + conn, bufrw, err := hj.Hijack() + runtimex.PanicOnError(err, "hj.Hijack failed") + bufrw.WriteString("HTTP/1.0 302 Found\r\n") + bufrw.WriteString("Location: /text\r\n\r\n") + bufrw.Flush() + conn.Close() + })) + defer srvr.Close() + txp := netxlite.NewHTTPTransportStdlib(model.DiscardLogger) + req, err := http.NewRequest("GET", srvr.URL, nil) + if err != nil { + t.Fatal(err) + } + resp, err := txp.RoundTrip(req) + if err != nil { + t.Fatal(err) + } + defer resp.Body.Close() + data, err := netxlite.ReadAllContext(req.Context(), resp.Body) + if err != nil { + t.Fatal(err) + } + t.Log(string(data)) + }) } func TestHTTP3Transport(t *testing.T) { diff --git a/internal/netxlite/iox.go b/internal/netxlite/iox.go index 53fa22b..1ae617a 100644 --- a/internal/netxlite/iox.go +++ b/internal/netxlite/iox.go @@ -2,6 +2,7 @@ package netxlite import ( "context" + "errors" "io" ) @@ -13,10 +14,18 @@ import ( // the long-running goroutine, close the connection // bound to the reader. Until such a connection is closed, // you're leaking the backround goroutine and doing I/O. +// +// As of Go 1.17.6, ReadAllContext additionally deals +// with wrapped io.EOF correctly, while io.ReadAll does +// not. See https://github.com/ooni/probe/issues/1965. func ReadAllContext(ctx context.Context, r io.Reader) ([]byte, error) { datach, errch := make(chan []byte, 1), make(chan error, 1) // buffers go func() { data, err := io.ReadAll(r) + if errors.Is(err, io.EOF) { + // See https://github.com/ooni/probe/issues/1965 + err = nil + } if err != nil { errch <- err return @@ -37,10 +46,18 @@ func ReadAllContext(ctx context.Context, r io.Reader) ([]byte, error) { // when the context expires. This function has the same // caveats of ReadAllContext regarding the temporary leaking // of the background I/O goroutine. +// +// As of Go 1.17.6, CopyContext additionally deals +// with wrapped io.EOF correctly, while io.Copy does +// not. See https://github.com/ooni/probe/issues/1965. func CopyContext(ctx context.Context, dst io.Writer, src io.Reader) (int64, error) { countch, errch := make(chan int64, 1), make(chan error, 1) // buffers go func() { count, err := io.Copy(dst, src) + if errors.Is(err, io.EOF) { + // See https://github.com/ooni/probe/issues/1965 + err = nil + } if err != nil { errch <- err return diff --git a/internal/netxlite/iox_test.go b/internal/netxlite/iox_test.go index 04f155c..ebf7e2b 100644 --- a/internal/netxlite/iox_test.go +++ b/internal/netxlite/iox_test.go @@ -24,6 +24,37 @@ func TestReadAllContext(t *testing.T) { } }) + t.Run("with success and wrapped io.EOF", func(t *testing.T) { + // See https://github.com/ooni/probe/issues/1965 + wg := &sync.WaitGroup{} + wg.Add(1) + r := &mocks.Reader{ + MockRead: func(b []byte) (int, error) { + defer wg.Done() + // "When Read encounters an error or end-of-file condition + // after successfully reading n > 0 bytes, it returns + // the number of bytes read. It may return the (non-nil) + // error from the same call or return the error (and n == 0) + // from a subsequent call."" + // + // See https://pkg.go.dev/io#Reader + // + // Note: Returning a wrapped error to ensure we address + // https://github.com/ooni/probe/issues/1965 + return len(b), NewErrWrapper(classifyGenericError, + ReadOperation, io.EOF) + }, + } + out, err := ReadAllContext(context.Background(), r) + if err != nil { + t.Fatal(err) + } + if len(out) <= 0 { + t.Fatal("we expected to see a positive number of bytes here") + } + wg.Wait() + }) + t.Run("with failure and background context", func(t *testing.T) { expected := errors.New("mocked error") r := &mocks.Reader{ @@ -123,6 +154,37 @@ func TestCopyContext(t *testing.T) { } }) + t.Run("with success and wrapped io.EOF", func(t *testing.T) { + // See https://github.com/ooni/probe/issues/1965 + wg := &sync.WaitGroup{} + wg.Add(1) + r := &mocks.Reader{ + MockRead: func(b []byte) (int, error) { + defer wg.Done() + // "When Read encounters an error or end-of-file condition + // after successfully reading n > 0 bytes, it returns + // the number of bytes read. It may return the (non-nil) + // error from the same call or return the error (and n == 0) + // from a subsequent call."" + // + // See https://pkg.go.dev/io#Reader + // + // Note: Returning a wrapped error to ensure we address + // https://github.com/ooni/probe/issues/1965 + return len(b), NewErrWrapper(classifyGenericError, + ReadOperation, io.EOF) + }, + } + out, err := CopyContext(context.Background(), io.Discard, r) + if err != nil { + t.Fatal(err) + } + if out <= 0 { + t.Fatal("we expected to see a positive number of bytes here") + } + wg.Wait() + }) + t.Run("with failure and background context", func(t *testing.T) { expected := errors.New("mocked error") r := &mocks.Reader{ diff --git a/script/nocopyreadall.bash b/script/nocopyreadall.bash new file mode 100755 index 0000000..0797655 --- /dev/null +++ b/script/nocopyreadall.bash @@ -0,0 +1,27 @@ +#!/bin/bash +set -euo pipefail +exitcode=0 +for file in $(find . -type f -name \*.go); do + if [ "$file" = "./internal/netxlite/iox.go" ]; then + # We're allowed to use ReadAll and Copy in this file to + # implement safer wrappers for these functions. + continue + fi + if grep -q 'io\.ReadAll' $file; then + echo "in $file: do not use io.ReadAll, use netxlite.ReadAllContext" 1>&2 + exitcode=1 + fi + if grep -q 'ioutil\.ReadAll' $file; then + echo "in $file: do not use ioutil.ReadAll, use netxlite.ReadAllContext" 1>&2 + exitcode=1 + fi + if grep -q 'io\.Copy' $file; then + echo "in $file: do not use io.Copy, use netxlite.CopyContext" 1>&2 + exitcode=1 + fi + if grep -q 'ioutil\.Copy' $file; then + echo "in $file: do not use ioutil.Copy, use netxlite.CopyContext" 1>&2 + exitcode=1 + fi +done +exit $exitcode