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

@@ -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

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) {
+	if err != nil {
-		t.Fatal("not the error we expected")
+		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) {
+	if !errors.Is(err, urlgetter.ErrHTTPRequestFailed) {
-		t.Fatal("not the error we expected")
+		t.Fatal("not the error we expected", err)
 	}
 }
 

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
 	}

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)))
 }

internal/engine/netx/httptransport/saver_internal_test.go

@@ -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)
-	}
-}

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)

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)
 }

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) {

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

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{

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