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