From 185a55026b6701e9f8363cbfe764bca503f4de95 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sun, 16 May 2021 00:28:12 +0200 Subject: [PATCH 1/4] rest: workaround for HTTP2 zero-length replies bug The golang http client does not return an error when a HTTP2 reply includes a non-zero content length but does not return any data at all. This scenario can occur e.g. when using rclone when a file stored in a backend seems to be accessible but then fails to download. --- internal/backend/rest/rest.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/internal/backend/rest/rest.go b/internal/backend/rest/rest.go index 44fcaead6..78cb6b10b 100644 --- a/internal/backend/rest/rest.go +++ b/internal/backend/rest/rest.go @@ -9,6 +9,7 @@ import ( "net/http" "net/url" "path" + "strconv" "strings" "golang.org/x/net/context/ctxhttp" @@ -246,6 +247,20 @@ func (b *Backend) openReader(ctx context.Context, h restic.Handle, length int, o return nil, errors.Errorf("unexpected HTTP response (%v): %v", resp.StatusCode, resp.Status) } + // workaround https://github.com/golang/go/issues/46071 + // see also https://forum.restic.net/t/http2-stream-closed-connection-reset-context-canceled/3743/10 + if resp.ContentLength == 0 && resp.ProtoMajor == 2 && resp.ProtoMinor == 0 { + if clens := resp.Header["Content-Length"]; len(clens) == 1 { + if cl, err := strconv.ParseUint(clens[0], 10, 63); err == nil { + resp.ContentLength = int64(cl) + } + if resp.ContentLength != 0 { + _ = resp.Body.Close() + return nil, errors.Errorf("unexpected EOF got 0 instead of %v bytes", resp.ContentLength) + } + } + } + return resp.Body, nil } From 097ed659b21b740de1b71515bf6fc95d5bf4f86a Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sat, 10 Jul 2021 16:57:13 +0200 Subject: [PATCH 2/4] rest: test that zero-length replies over HTTP2 work correctly The first test function ensures that the workaround works as expected. And the second test function is intended to fail as soon as the issue has been fixed in golang to allow us to eventually remove the workaround. --- internal/backend/rest/rest_int_go114_test.go | 94 ++++++++++++++++++++ 1 file changed, 94 insertions(+) create mode 100644 internal/backend/rest/rest_int_go114_test.go diff --git a/internal/backend/rest/rest_int_go114_test.go b/internal/backend/rest/rest_int_go114_test.go new file mode 100644 index 000000000..036a136e1 --- /dev/null +++ b/internal/backend/rest/rest_int_go114_test.go @@ -0,0 +1,94 @@ +// +build go1.14 + +package rest_test + +import ( + "context" + "io" + "io/ioutil" + "net/http" + "net/http/httptest" + "net/url" + "testing" + + "github.com/restic/restic/internal/backend/rest" + "github.com/restic/restic/internal/restic" +) + +func TestZeroLengthRead(t *testing.T) { + // Test workaround for https://github.com/golang/go/issues/46071. Can be removed once this is fixed in Go + // and the minimum golang version supported by restic includes the fix. + numRequests := 0 + srv := httptest.NewUnstartedServer(http.HandlerFunc(func(res http.ResponseWriter, req *http.Request) { + numRequests++ + t.Logf("req %v %v", req.Method, req.URL.Path) + if req.Method == "GET" { + res.Header().Set("Content-Length", "42") + // Now the handler fails for some reason and is unable to send data + return + } + + t.Errorf("unhandled request %v %v", req.Method, req.URL.Path) + })) + srv.EnableHTTP2 = true + srv.StartTLS() + defer srv.Close() + + srvURL, err := url.Parse(srv.URL) + if err != nil { + t.Fatal(err) + } + + cfg := rest.Config{ + Connections: 5, + URL: srvURL, + } + be, err := rest.Open(cfg, srv.Client().Transport) + if err != nil { + t.Fatal(err) + } + defer func() { + err = be.Close() + if err != nil { + t.Fatal(err) + } + }() + + err = be.Load(context.TODO(), restic.Handle{Type: restic.ConfigFile}, 0, 0, func(rd io.Reader) error { + _, err := ioutil.ReadAll(rd) + if err == nil { + t.Fatal("ReadAll should have returned an 'Unexpected EOF' error") + } + return nil + }) + if err == nil { + t.Fatal("Got no unexpected EOF error") + } +} + +func TestGolangZeroLengthRead(t *testing.T) { + // This test is intended to fail once the underlying issue has been fixed in Go + ts := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Length", "42") + // Now the handler fails for some reason and is unable to send data + })) + ts.EnableHTTP2 = true + ts.StartTLS() + defer ts.Close() + + res, err := ts.Client().Get(ts.URL) + if err != nil { + t.Fatal(err) + } + _, err = ioutil.ReadAll(res.Body) + defer func() { + err = res.Body.Close() + if err != nil { + t.Fatal(err) + } + }() + if err != nil { + // This should fail with an 'Unexpected EOF' error + t.Fatal(err) + } +} From 7d28006e2e22fea6c11a95e1179b29cd3f51c6af Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sat, 10 Jul 2021 22:39:01 +0200 Subject: [PATCH 3/4] Add changelog --- changelog/unreleased/issue-2742 | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 changelog/unreleased/issue-2742 diff --git a/changelog/unreleased/issue-2742 b/changelog/unreleased/issue-2742 new file mode 100644 index 000000000..72dac6de8 --- /dev/null +++ b/changelog/unreleased/issue-2742 @@ -0,0 +1,15 @@ +Bugfix: Improve error handling for rclone and rest backend over HTTP2 + +When retrieving data from the rclone / rest backend while also using HTTP2 +restic did not detect when no data was returned at all. This could cause +for example the `check` command to report the following error: +``` +Pack ID does not match, want xxxxxxxx, got e3b0c442 +``` + +This has been fixed by correctly detecting the incomplete download and +retrying the download. + +https://github.com/restic/restic/issues/2742 +https://github.com/restic/restic/pull/3453 +https://forum.restic.net/t/http2-stream-closed-connection-reset-context-canceled/3743/10 From d8ea10db8c020ad17a68dfe27f301f2b8b0f6073 Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Fri, 30 Jul 2021 10:17:28 +0200 Subject: [PATCH 4/4] rest: Rework handling HTTP2 zero-length replies bug Add comment that the check is based on the stdlib HTTP2 client. Refactor the checks into a function. Return an error if the value in the Content-Length header cannot be parsed. --- internal/backend/rest/rest.go | 53 ++++++++++++++++++++++++++++------- 1 file changed, 43 insertions(+), 10 deletions(-) diff --git a/internal/backend/rest/rest.go b/internal/backend/rest/rest.go index 78cb6b10b..55732e871 100644 --- a/internal/backend/rest/rest.go +++ b/internal/backend/rest/rest.go @@ -7,6 +7,7 @@ import ( "io" "io/ioutil" "net/http" + "net/textproto" "net/url" "path" "strconv" @@ -198,6 +199,44 @@ func (b *Backend) Load(ctx context.Context, h restic.Handle, length int, offset return err } +// checkContentLength returns an error if the server returned a value in the +// Content-Length header in an HTTP2 connection, but closed the connection +// before any data was sent. +// +// This is a workaround for https://github.com/golang/go/issues/46071 +// +// See also https://forum.restic.net/t/http2-stream-closed-connection-reset-context-canceled/3743/10 +func checkContentLength(resp *http.Response) error { + // the following code is based on + // https://github.com/golang/go/blob/b7a85e0003cedb1b48a1fd3ae5b746ec6330102e/src/net/http/h2_bundle.go#L8646 + + if resp.ContentLength != 0 { + return nil + } + + if resp.ProtoMajor != 2 && resp.ProtoMinor != 0 { + return nil + } + + if len(resp.Header[textproto.CanonicalMIMEHeaderKey("Content-Length")]) != 1 { + return nil + } + + // make sure that if the server returned a content length and we can + // parse it, it is really zero, otherwise return an error + contentLength := resp.Header.Get("Content-Length") + cl, err := strconv.ParseUint(contentLength, 10, 63) + if err != nil { + return fmt.Errorf("unable to parse Content-Length %q: %w", contentLength, err) + } + + if cl != 0 { + return errors.Errorf("unexpected EOF: got 0 instead of %v bytes", cl) + } + + return nil +} + func (b *Backend) openReader(ctx context.Context, h restic.Handle, length int, offset int64) (io.ReadCloser, error) { debug.Log("Load %v, length %v, offset %v", h, length, offset) if err := h.Valid(); err != nil { @@ -249,16 +288,10 @@ func (b *Backend) openReader(ctx context.Context, h restic.Handle, length int, o // workaround https://github.com/golang/go/issues/46071 // see also https://forum.restic.net/t/http2-stream-closed-connection-reset-context-canceled/3743/10 - if resp.ContentLength == 0 && resp.ProtoMajor == 2 && resp.ProtoMinor == 0 { - if clens := resp.Header["Content-Length"]; len(clens) == 1 { - if cl, err := strconv.ParseUint(clens[0], 10, 63); err == nil { - resp.ContentLength = int64(cl) - } - if resp.ContentLength != 0 { - _ = resp.Body.Close() - return nil, errors.Errorf("unexpected EOF got 0 instead of %v bytes", resp.ContentLength) - } - } + err = checkContentLength(resp) + if err != nil { + _ = resp.Body.Close() + return nil, err } return resp.Body, nil