From fff47b8cfad937ffcbe55f73404d87c01e8cedc2 Mon Sep 17 00:00:00 2001 From: Oliver Calder Date: Mon, 27 Jan 2025 20:15:42 -0600 Subject: [PATCH 01/13] store: add functions to download snap icons These new functions, `downloadIconImpl` and `DownloadIcon`, mirror their non-icon counterparts `downloadImpl` and `Download`, except that because snap icons are small, simple assets on a fileserver, there is no need for snap-specific headers, authentication, partial downloads, or caching. These will be used in the future in order to download a snap's icon whenever the new `.snap` blob is downloaded for that snap and linked to a snap icons directory so that the snapd API can serve snap icons from local icon files, rather than returning an error if the snap does not ship an icon at `meta/gui/icon.*`. Signed-off-by: Oliver Calder --- store/download_test.go | 106 +++++++++++++++++++++++++ store/export_test.go | 6 ++ store/store.go | 25 ++++++ store/store_download.go | 131 +++++++++++++++++++++++++++++++ store/store_download_test.go | 148 +++++++++++++++++++++++++++++++++++ 5 files changed, 416 insertions(+) diff --git a/store/download_test.go b/store/download_test.go index c092cc1af87..0f739dd15bb 100644 --- a/store/download_test.go +++ b/store/download_test.go @@ -708,3 +708,109 @@ func (s *downloadSuite) TestActualDownloadRateLimited(c *C) { c.Check(buf.String(), Equals, canary) c.Check(ratelimitReaderUsed, Equals, true) } + +func (s *downloadSuite) TestActualDownloadIcon(c *C) { + n := 0 + mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + c.Check(r.Header.Get("Snap-CDN"), Equals, "") + c.Check(r.Header.Get("Snap-Device-Location"), Equals, "") + c.Check(r.Header.Get("Snap-Refresh-Reason"), Equals, "") + n++ + io.WriteString(w, "response-data") + })) + c.Assert(mockServer, NotNil) + defer mockServer.Close() + + var buf SillyBuffer + err := store.DownloadIconImpl(context.TODO(), "foo", mockServer.URL, &buf) + c.Assert(err, IsNil) + c.Check(buf.String(), Equals, "response-data") + c.Check(n, Equals, 1) +} + +func (s *downloadSuite) TestDownloadIconCancellation(c *C) { + // the channel used by mock server to request cancellation from the test + syncCh := make(chan struct{}) + + n := 0 + mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + n++ + io.WriteString(w, "foo") + syncCh <- struct{}{} + io.WriteString(w, "bar") + time.Sleep(10 * time.Millisecond) + })) + c.Assert(mockServer, NotNil) + defer mockServer.Close() + + ctx, cancel := context.WithCancel(context.Background()) + + result := make(chan string) + go func() { + var buf SillyBuffer + err := store.DownloadIconImpl(ctx, "foo", mockServer.URL, &buf) + result <- err.Error() + close(result) + }() + + <-syncCh + cancel() + + err := <-result + c.Check(n, Equals, 1) + c.Assert(err, Equals, "the download has been cancelled: context canceled") +} + +func (s *downloadSuite) TestActualDownloadIcon404(c *C) { + n := 0 + mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + n++ + w.WriteHeader(404) + })) + c.Assert(mockServer, NotNil) + defer mockServer.Close() + + var buf SillyBuffer + err := store.DownloadIconImpl(context.TODO(), "foo", mockServer.URL, &buf) + c.Assert(err, NotNil) + c.Assert(err, FitsTypeOf, &store.DownloadError{}) + c.Check(err.(*store.DownloadError).Code, Equals, 404) + c.Check(n, Equals, 1) +} + +func (s *downloadSuite) TestActualDownloadIcon500(c *C) { + n := 0 + mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + n++ + w.WriteHeader(500) + })) + c.Assert(mockServer, NotNil) + defer mockServer.Close() + + var buf SillyBuffer + err := store.DownloadIconImpl(context.TODO(), "foo", mockServer.URL, &buf) + c.Assert(err, NotNil) + c.Assert(err, FitsTypeOf, &store.DownloadError{}) + c.Check(err.(*store.DownloadError).Code, Equals, 500) + c.Check(n, Equals, 5) +} + +func (s *downloadSuite) TestActualDownloadIcon500Once(c *C) { + n := 0 + mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + n++ + if n == 1 { + w.WriteHeader(500) + } else { + io.WriteString(w, "response-data") + } + })) + c.Assert(mockServer, NotNil) + defer mockServer.Close() + + var buf SillyBuffer + err := store.DownloadIconImpl(context.TODO(), "foo", mockServer.URL, &buf) + c.Assert(err, IsNil) + c.Check(buf.String(), Equals, "response-data") + c.Check(n, Equals, 2) +} diff --git a/store/export_test.go b/store/export_test.go index e0938b766d5..e279441ffc5 100644 --- a/store/export_test.go +++ b/store/export_test.go @@ -42,6 +42,8 @@ var ( ApiURL = apiURL Download = download + DownloadIconImpl = downloadIcon + ApplyDelta = applyDelta AuthLocation = authLocation @@ -149,6 +151,10 @@ func MockDownload(f func(ctx context.Context, name, sha3_384, downloadURL string } } +func MockDownloadIcon(f func(ctx context.Context, name, downloadURL string, w io.ReadWriteSeeker) error) (restore func()) { + return testutil.Mock(&downloadIcon, f) +} + func MockDoDownloadReq(f func(ctx context.Context, storeURL *url.URL, cdnHeader string, resume int64, s *Store, user *auth.UserState) (*http.Response, error)) (restore func()) { orig := doDownloadReq doDownloadReq = f diff --git a/store/store.go b/store/store.go index 956d71d1eed..944166f53c2 100644 --- a/store/store.go +++ b/store/store.go @@ -766,6 +766,31 @@ func (s *Store) doRequest(ctx context.Context, client *http.Client, reqOptions * } } +func doIconRequest(ctx context.Context, client *http.Client, reqOptions *requestOptions) (*http.Response, error) { + var body io.Reader // empty body + req, err := http.NewRequest(reqOptions.Method, reqOptions.URL.String(), body) + if err != nil { + return nil, err + } + if reqOptions.ContentType != "" { + req.Header.Set("Content-Type", reqOptions.ContentType) + } + for header, value := range reqOptions.ExtraHeaders { + req.Header.Set(header, value) + } + + if ctx != nil { + req = req.WithContext(ctx) + } + + resp, err := client.Do(req) + if err != nil { + return nil, err + } + + return resp, err +} + func (s *Store) buildLocationString() (string, error) { if s.dauthCtx == nil { return "", nil diff --git a/store/store_download.go b/store/store_download.go index 430afe5613d..c3974b37154 100644 --- a/store/store_download.go +++ b/store/store_download.go @@ -308,6 +308,59 @@ func (s *Store) Download(ctx context.Context, name string, targetPath string, do return s.cacher.Put(downloadInfo.Sha3_384, targetPath) } +// DownloadIcon downloads the icon for the snap from the given download URL to +// the given target path. Snap icons are small (<256kB) files served from an +// ordinary unauthenticated file server, so this does not require store +// authentication or user state, nor a progress bar. They are also not revision- +// specific, and do not use a download cache. +func DownloadIcon(ctx context.Context, name string, targetPath string, downloadURL string) error { + if err := os.MkdirAll(filepath.Dir(targetPath), 0o755); err != nil { + return err + } + + // Download icon to a temporary file location, since there may be an active + // snap icon hard-linked to targetPath. If download fails, don't want to + // corrupt the existing icon file contents. Instead, move partialPath to + // targetPath once the download succeeds, thus leaving any previous icon + // linked to the original unchanged icon contents, until a future task + // re-links to the new contents. + partialPath := targetPath + ".partial" + + // Open the new partial file and truncate it, since any existing file may + // be old, from a previous version of the snap icon. Don't use `resume` as + // in `Download()`; always start over. + w, err := os.OpenFile(partialPath, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0o600) + if err != nil { + return err + } + defer func() { + if cerr := w.Close(); cerr != nil && err == nil { + err = cerr + } + if err == nil { + return + } + os.Remove(w.Name()) + }() + logger.Debugf("Starting download of %q.", partialPath) + + err = downloadIcon(ctx, name, downloadURL, w) + if err != nil { + logger.Debugf("download of %q failed: %#v", downloadURL, err) + return err + } + + if err = os.Rename(w.Name(), targetPath); err != nil { + return err + } + + if err = w.Sync(); err != nil { + return err + } + + return nil +} + func downloadReqOpts(storeURL *url.URL, cdnHeader string, opts *DownloadOptions) *requestOptions { reqOptions := requestOptions{ Method: "GET", @@ -600,6 +653,84 @@ func downloadImpl(ctx context.Context, name, sha3_384, downloadURL string, user return finalErr } +var downloadIcon = downloadIconImpl + +// downloadIconImpl writes an http.Request which does not require authentication +// or a progress.Meter. +func downloadIconImpl(ctx context.Context, name, downloadURL string, w io.ReadWriteSeeker) error { + url, err := url.Parse(downloadURL) + if err != nil { + return err + } + + var finalErr error + startTime := time.Now() + for attempt := retry.Start(downloadRetryStrategy, nil); attempt.Next(); { + // Don't need any special snap-specific headers or download options + cdnHeader := "" + dlOpts := &DownloadOptions{} + reqOptions := downloadReqOpts(url, cdnHeader, dlOpts) + + httputil.MaybeLogRetryAttempt(url.String(), attempt, startTime) + + if cancelled(ctx) { + return fmt.Errorf("the download has been cancelled: %s", ctx.Err()) + } + + cli := httputil.NewHTTPClient(nil) + var resp *http.Response + resp, finalErr = doIconRequest(ctx, cli, reqOptions) + if cancelled(ctx) { + return fmt.Errorf("the download has been cancelled: %s", ctx.Err()) + } + if finalErr != nil { + if httputil.ShouldRetryAttempt(attempt, finalErr) { + continue + } + break + } + + if httputil.ShouldRetryHttpResponse(attempt, resp) { + resp.Body.Close() + continue + } + + // XXX: we're inside retry loop, so this will be closed only on return. + defer resp.Body.Close() + + switch resp.StatusCode { + case 200: // OK + default: + return &DownloadError{Code: resp.StatusCode, URL: resp.Request.URL} + } + if resp.ContentLength == 0 { + logger.Debugf("Unexpected Content-Length: 0 for %s", downloadURL) + } else { + logger.Debugf("Download size for %s: %d", downloadURL, resp.ContentLength) + } + + _, finalErr = io.Copy(w, resp.Body) + + if cancelled(ctx) { + return fmt.Errorf("the download has been cancelled: %s", ctx.Err()) + } + + if finalErr != nil { + if httputil.ShouldRetryAttempt(attempt, finalErr) { + // XXX: is this correct, without resume? Or should we just + // error here since we're not doing Range requests? + continue + } + break + } + break + } + if finalErr == nil { + logger.Debugf("Icon download succeeded") + } + return finalErr +} + // DownloadStream will copy the snap from the request to the io.Reader func (s *Store) DownloadStream(ctx context.Context, name string, downloadInfo *snap.DownloadInfo, resume int64, user *auth.UserState) (io.ReadCloser, int, error) { // most other store network operations use s.endpointURL, which returns an diff --git a/store/store_download_test.go b/store/store_download_test.go index 2b06560e7b9..b1d931ad485 100644 --- a/store/store_download_test.go +++ b/store/store_download_test.go @@ -1252,3 +1252,151 @@ func (s *storeDownloadSuite) TestDownloadInfiniteRedirect(c *C) { err := s.store.Download(s.ctx, "foo", targetFn, &snap.DownloadInfo, nil, s.user, nil) c.Assert(err, ErrorMatches, fmt.Sprintf("Get %q: stopped after 10 redirects", mockServer.URL)) } + +func (s *storeDownloadSuite) TestDownloadIconOK(c *C) { + expectedURL := "URL" + expectedContent := []byte("I was downloaded") + + restore := store.MockDownloadIcon(func(ctx context.Context, name, url string, w io.ReadWriteSeeker) error { + c.Check(url, Equals, expectedURL) + w.Write(expectedContent) + return nil + }) + defer restore() + + path := filepath.Join(c.MkDir(), "downloaded-file") + err := store.DownloadIcon(s.ctx, "foo", path, expectedURL) + c.Assert(err, IsNil) + defer os.Remove(path) + + c.Assert(path, testutil.FileEquals, expectedContent) +} + +func (s *storeDownloadSuite) TestDownloadIconDoesNotOverwriteLinks(c *C) { + expectedURL := "URL" + oldContent := []byte("I was already here") + newContent := []byte("I was downloaded") + + restore := store.MockDownloadIcon(func(ctx context.Context, name, url string, w io.ReadWriteSeeker) error { + c.Check(url, Equals, expectedURL) + w.Write(newContent) + return nil + }) + defer restore() + + path := filepath.Join(c.MkDir(), "downloaded-file") + linkPath := path + "-existing" + + // Create an existing file at the path + err := os.MkdirAll(filepath.Dir(path), 0o755) + c.Assert(err, IsNil) + err = os.WriteFile(path, oldContent, 0o600) + c.Assert(err, IsNil) + // Create a hard link to the existing file + err = os.Link(path, linkPath) + c.Assert(err, IsNil) + + err = store.DownloadIcon(s.ctx, "foo", path, expectedURL) + c.Assert(err, IsNil) + defer os.Remove(path) + + c.Assert(path, testutil.FileEquals, newContent) + // Check that the contents of the existing hard-linked file were not overwritten + c.Assert(linkPath, testutil.FileEquals, oldContent) +} + +func (s *storeDownloadSuite) TestDownloadIconFails(c *C) { + fakeName := "foo" + fakeURL := "URL" + + var tmpfile *os.File + restore := store.MockDownloadIcon(func(ctx context.Context, name, url string, w io.ReadWriteSeeker) error { + c.Assert(name, Equals, fakeName) + c.Assert(url, Equals, fakeURL) + tmpfile = w.(*os.File) + return fmt.Errorf("uh, it failed") + }) + defer restore() + + // simulate a failed download + path := filepath.Join(c.MkDir(), "downloaded-file") + err := store.DownloadIcon(s.ctx, fakeName, path, fakeURL) + c.Assert(err, ErrorMatches, "uh, it failed") + // ... and ensure that the tempfile is removed + c.Assert(osutil.FileExists(tmpfile.Name()), Equals, false) + // ... and not because it succeeded either + c.Assert(osutil.FileExists(path), Equals, false) +} + +func (s *storeDownloadSuite) TestDownloadIconFailsDoesNotLeavePartial(c *C) { + fakeName := "foo" + fakeURL := "URL" + + var tmpfile *os.File + restore := store.MockDownloadIcon(func(ctx context.Context, name, url string, w io.ReadWriteSeeker) error { + c.Assert(name, Equals, fakeName) + c.Assert(url, Equals, fakeURL) + tmpfile = w.(*os.File) + w.Write([]byte{'X'}) // so it's not empty + return fmt.Errorf("uh, it failed") + }) + defer restore() + + // simulate a failed download + path := filepath.Join(c.MkDir(), "downloaded-file") + err := store.DownloadIcon(s.ctx, fakeName, path, fakeURL) + c.Assert(err, ErrorMatches, "uh, it failed") + // ... and ensure that the tempfile is removed + c.Assert(osutil.FileExists(tmpfile.Name()), Equals, false) + // ... and the target path isn't there + c.Assert(osutil.FileExists(path), Equals, false) +} + +func (s *storeDownloadSuite) TestDownloadIconSyncFails(c *C) { + fakeName := "foo" + fakeURL := "URL" + + var tmpfile *os.File + restore := store.MockDownloadIcon(func(ctx context.Context, name, url string, w io.ReadWriteSeeker) error { + c.Assert(name, Equals, fakeName) + c.Assert(url, Equals, fakeURL) + tmpfile = w.(*os.File) + w.Write([]byte("sync will fail")) + err := tmpfile.Close() + c.Assert(err, IsNil) + return nil + }) + defer restore() + + // simulate a failed sync + path := filepath.Join(c.MkDir(), "downloaded-file") + err := store.DownloadIcon(s.ctx, fakeName, path, fakeURL) + c.Assert(err, ErrorMatches, "(sync|fsync:) .*") + // ... and ensure that the tempfile is removed + c.Assert(osutil.FileExists(tmpfile.Name()), Equals, false) + // ... because it's been renamed to the target path already + c.Assert(osutil.FileExists(path), Equals, true) +} + +func (s *storeDownloadSuite) TestDownloadIconInfiniteRedirect(c *C) { + n := 0 + var mockServer *httptest.Server + + mockServer = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // n = 0 -> initial request + // n = 10 -> max redirects + // n = 11 -> exceeded max redirects + c.Assert(n, testutil.IntNotEqual, 11) + n++ + http.Redirect(w, r, mockServer.URL, 302) + })) + c.Assert(mockServer, NotNil) + defer mockServer.Close() + + fakeName := "foo" + fakeURL := mockServer.URL + + targetPath := filepath.Join(c.MkDir(), "foo.icon") + err := store.DownloadIcon(s.ctx, fakeName, targetPath, fakeURL) + c.Assert(err, ErrorMatches, fmt.Sprintf("Get %q: stopped after 10 redirects", fakeURL)) +} From fb306be7f8e782496acb138dfd607cd5e9b39b3e Mon Sep 17 00:00:00 2001 From: Oliver Calder Date: Tue, 28 Jan 2025 10:31:48 -0600 Subject: [PATCH 02/13] store: simplify doIconRequest to remove unused request options Signed-off-by: Oliver Calder --- store/store.go | 12 ++++-------- store/store_download.go | 14 +++++--------- 2 files changed, 9 insertions(+), 17 deletions(-) diff --git a/store/store.go b/store/store.go index 944166f53c2..e9485b10421 100644 --- a/store/store.go +++ b/store/store.go @@ -766,18 +766,14 @@ func (s *Store) doRequest(ctx context.Context, client *http.Client, reqOptions * } } -func doIconRequest(ctx context.Context, client *http.Client, reqOptions *requestOptions) (*http.Response, error) { +// doIconRequest does an unauthenticated GET request to the given URL. +func doIconRequest(ctx context.Context, client *http.Client, iconURL *url.URL) (*http.Response, error) { + method := "GET" var body io.Reader // empty body - req, err := http.NewRequest(reqOptions.Method, reqOptions.URL.String(), body) + req, err := http.NewRequest(method, iconURL.String(), body) if err != nil { return nil, err } - if reqOptions.ContentType != "" { - req.Header.Set("Content-Type", reqOptions.ContentType) - } - for header, value := range reqOptions.ExtraHeaders { - req.Header.Set(header, value) - } if ctx != nil { req = req.WithContext(ctx) diff --git a/store/store_download.go b/store/store_download.go index c3974b37154..c3412774968 100644 --- a/store/store_download.go +++ b/store/store_download.go @@ -658,7 +658,7 @@ var downloadIcon = downloadIconImpl // downloadIconImpl writes an http.Request which does not require authentication // or a progress.Meter. func downloadIconImpl(ctx context.Context, name, downloadURL string, w io.ReadWriteSeeker) error { - url, err := url.Parse(downloadURL) + iconURL, err := url.Parse(downloadURL) if err != nil { return err } @@ -666,20 +666,16 @@ func downloadIconImpl(ctx context.Context, name, downloadURL string, w io.ReadWr var finalErr error startTime := time.Now() for attempt := retry.Start(downloadRetryStrategy, nil); attempt.Next(); { - // Don't need any special snap-specific headers or download options - cdnHeader := "" - dlOpts := &DownloadOptions{} - reqOptions := downloadReqOpts(url, cdnHeader, dlOpts) - - httputil.MaybeLogRetryAttempt(url.String(), attempt, startTime) + httputil.MaybeLogRetryAttempt(iconURL.String(), attempt, startTime) if cancelled(ctx) { return fmt.Errorf("the download has been cancelled: %s", ctx.Err()) } - cli := httputil.NewHTTPClient(nil) + clientOpts := &httputil.ClientOptions{} // don't need any options + cli := httputil.NewHTTPClient(clientOpts) var resp *http.Response - resp, finalErr = doIconRequest(ctx, cli, reqOptions) + resp, finalErr = doIconRequest(ctx, cli, iconURL) if cancelled(ctx) { return fmt.Errorf("the download has been cancelled: %s", ctx.Err()) } From 774ae5d7e2f4324fd9a64dd08fe05533ccd7078e Mon Sep 17 00:00:00 2001 From: Oliver Calder Date: Thu, 30 Jan 2025 12:48:21 -0600 Subject: [PATCH 03/13] store: enforce snap icon size limit during download Signed-off-by: Oliver Calder --- store/store_download.go | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/store/store_download.go b/store/store_download.go index c3412774968..ad956bb20a9 100644 --- a/store/store_download.go +++ b/store/store_download.go @@ -699,13 +699,17 @@ func downloadIconImpl(ctx context.Context, name, downloadURL string, w io.ReadWr default: return &DownloadError{Code: resp.StatusCode, URL: resp.Request.URL} } - if resp.ContentLength == 0 { - logger.Debugf("Unexpected Content-Length: 0 for %s", downloadURL) + + const maxIconFilesize int64 = 300000 + + if resp.ContentLength == 0 || resp.ContentLength > maxIconFilesize { + return fmt.Errorf("unsupported Content-Length for %s (must be nonzero and <%dB): %d", downloadURL, maxIconFilesize, resp.ContentLength) } else { logger.Debugf("Download size for %s: %d", downloadURL, resp.ContentLength) } - _, finalErr = io.Copy(w, resp.Body) + var bytesWritten int64 + bytesWritten, finalErr = io.Copy(w, resp.Body) if cancelled(ctx) { return fmt.Errorf("the download has been cancelled: %s", ctx.Err()) @@ -715,10 +719,14 @@ func downloadIconImpl(ctx context.Context, name, downloadURL string, w io.ReadWr if httputil.ShouldRetryAttempt(attempt, finalErr) { // XXX: is this correct, without resume? Or should we just // error here since we're not doing Range requests? + // And should we seek w back to 0? continue } break } + if bytesWritten > maxIconFilesize { + return fmt.Errorf("the download size exceeded the max icon filesize (%dB), despite that the reported Content-Length was %d: %d", maxIconFilesize, resp.ContentLength, bytesWritten) + } break } if finalErr == nil { From 5658e581eeb76882a99ca78f7bb617ff21e88198 Mon Sep 17 00:00:00 2001 From: Oliver Calder Date: Thu, 30 Jan 2025 15:40:43 -0600 Subject: [PATCH 04/13] store: clarify sync/close/rename behavior when downloading snap icon Signed-off-by: Oliver Calder --- store/store_download.go | 24 +++++++++++++++--------- store/store_download_test.go | 33 ++++++++++++++++++++++++++++----- 2 files changed, 43 insertions(+), 14 deletions(-) diff --git a/store/store_download.go b/store/store_download.go index ad956bb20a9..02eb07ac5d6 100644 --- a/store/store_download.go +++ b/store/store_download.go @@ -334,12 +334,16 @@ func DownloadIcon(ctx context.Context, name string, targetPath string, downloadU return err } defer func() { - if cerr := w.Close(); cerr != nil && err == nil { - err = cerr - } - if err == nil { - return - } + // Unconditionally close the file, whether it's been closed before or + // not. If it has been closed before, close will return an error, which + // we ignore. If it hasn't been called before, then an error has already + // occurred, and we don't care about any new error from calling Close(). + w.Close() + // Unconditionally remove the .partial file, since w.Name() is the name + // with which the file was originally opened. If it still exists, then + // an error occurred, and we want to remove it. If it doesn't still + // exist, then it was successfully renamed, so it's harmless to attempt + // to remove the original .partial filename, and we ignore the error. os.Remove(w.Name()) }() logger.Debugf("Starting download of %q.", partialPath) @@ -350,15 +354,17 @@ func DownloadIcon(ctx context.Context, name string, targetPath string, downloadU return err } - if err = os.Rename(w.Name(), targetPath); err != nil { + if err = w.Sync(); err != nil { return err } - if err = w.Sync(); err != nil { + if err = w.Close(); err != nil { return err } - return nil + // Only rename once we know all is well, so we don't want to have any side + // effects on any existing snap icon if an error occurs. + return os.Rename(w.Name(), targetPath) } func downloadReqOpts(storeURL *url.URL, cdnHeader string, opts *DownloadOptions) *requestOptions { diff --git a/store/store_download_test.go b/store/store_download_test.go index b1d931ad485..dc6d7af6884 100644 --- a/store/store_download_test.go +++ b/store/store_download_test.go @@ -1352,10 +1352,36 @@ func (s *storeDownloadSuite) TestDownloadIconFailsDoesNotLeavePartial(c *C) { c.Assert(osutil.FileExists(path), Equals, false) } -func (s *storeDownloadSuite) TestDownloadIconSyncFails(c *C) { +func (s *storeDownloadSuite) TestDownloadIconSyncFailsWithExisting(c *C) { fakeName := "foo" + fakePath := filepath.Join(c.MkDir(), "downloaded-file") fakeURL := "URL" + // Create an existing file at the path + oldContent := []byte("I was already here") + err := os.MkdirAll(filepath.Dir(fakePath), 0o577) + c.Assert(err, IsNil) + err = os.WriteFile(fakePath, oldContent, 0o600) + c.Assert(err, IsNil) + + s.testDownloadIconSyncFailsGeneric(c, fakeName, fakePath, fakeURL) + + // Check that the existing file contents remain unchanged + c.Assert(fakePath, testutil.FileEquals, oldContent) +} + +func (s *storeDownloadSuite) TestDownloadIconSyncFailsWithoutExisting(c *C) { + fakeName := "foo" + fakePath := filepath.Join(c.MkDir(), "downloaded-file") + fakeURL := "URL" + + s.testDownloadIconSyncFailsGeneric(c, fakeName, fakePath, fakeURL) + + // Check that the file was not renamed to fakePath + c.Assert(osutil.FileExists(fakePath), Equals, false) +} + +func (s *storeDownloadSuite) testDownloadIconSyncFailsGeneric(c *C, fakeName, fakePath, fakeURL string) { var tmpfile *os.File restore := store.MockDownloadIcon(func(ctx context.Context, name, url string, w io.ReadWriteSeeker) error { c.Assert(name, Equals, fakeName) @@ -1369,13 +1395,10 @@ func (s *storeDownloadSuite) TestDownloadIconSyncFails(c *C) { defer restore() // simulate a failed sync - path := filepath.Join(c.MkDir(), "downloaded-file") - err := store.DownloadIcon(s.ctx, fakeName, path, fakeURL) + err := store.DownloadIcon(s.ctx, fakeName, fakePath, fakeURL) c.Assert(err, ErrorMatches, "(sync|fsync:) .*") // ... and ensure that the tempfile is removed c.Assert(osutil.FileExists(tmpfile.Name()), Equals, false) - // ... because it's been renamed to the target path already - c.Assert(osutil.FileExists(path), Equals, true) } func (s *storeDownloadSuite) TestDownloadIconInfiniteRedirect(c *C) { From 7b20bc205e9433961613ffc64e74eee4ff18209d Mon Sep 17 00:00:00 2001 From: Oliver Calder Date: Thu, 30 Jan 2025 15:43:20 -0600 Subject: [PATCH 05/13] store: simplify doIconRequest further Signed-off-by: Oliver Calder --- store/store.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/store/store.go b/store/store.go index e9485b10421..6d1b2031bf1 100644 --- a/store/store.go +++ b/store/store.go @@ -768,17 +768,12 @@ func (s *Store) doRequest(ctx context.Context, client *http.Client, reqOptions * // doIconRequest does an unauthenticated GET request to the given URL. func doIconRequest(ctx context.Context, client *http.Client, iconURL *url.URL) (*http.Response, error) { - method := "GET" var body io.Reader // empty body - req, err := http.NewRequest(method, iconURL.String(), body) + req, err := http.NewRequestWithContext(ctx, "GET", iconURL.String(), body) if err != nil { return nil, err } - if ctx != nil { - req = req.WithContext(ctx) - } - resp, err := client.Do(req) if err != nil { return nil, err From 9972463dde1cdb683dd4e77f436d150bc0071972 Mon Sep 17 00:00:00 2001 From: Oliver Calder Date: Mon, 3 Feb 2025 09:32:01 -0600 Subject: [PATCH 06/13] store: use CopyN when writing icon file contents Signed-off-by: Oliver Calder --- store/store_download.go | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/store/store_download.go b/store/store_download.go index 02eb07ac5d6..063cc38673f 100644 --- a/store/store_download.go +++ b/store/store_download.go @@ -714,8 +714,7 @@ func downloadIconImpl(ctx context.Context, name, downloadURL string, w io.ReadWr logger.Debugf("Download size for %s: %d", downloadURL, resp.ContentLength) } - var bytesWritten int64 - bytesWritten, finalErr = io.Copy(w, resp.Body) + _, finalErr = io.CopyN(w, resp.Body, resp.ContentLength) if cancelled(ctx) { return fmt.Errorf("the download has been cancelled: %s", ctx.Err()) @@ -723,16 +722,13 @@ func downloadIconImpl(ctx context.Context, name, downloadURL string, w io.ReadWr if finalErr != nil { if httputil.ShouldRetryAttempt(attempt, finalErr) { - // XXX: is this correct, without resume? Or should we just - // error here since we're not doing Range requests? - // And should we seek w back to 0? + // XXX: is this correct, without resume? ShouldRetryAttempt + // will return true on io.EOF errors. + // And if so, should we seek w back to 0 in the writer? continue } break } - if bytesWritten > maxIconFilesize { - return fmt.Errorf("the download size exceeded the max icon filesize (%dB), despite that the reported Content-Length was %d: %d", maxIconFilesize, resp.ContentLength, bytesWritten) - } break } if finalErr == nil { From 995ca513e3a53266c4eed76cbfc652c381b8ab8e Mon Sep 17 00:00:00 2001 From: Oliver Calder Date: Tue, 4 Feb 2025 10:46:58 -0600 Subject: [PATCH 07/13] store: use AtomicFile when downloading snap icons Signed-off-by: Oliver Calder --- store/store_download.go | 58 +++++++++++------------------------- store/store_download_test.go | 20 ++++++------- 2 files changed, 28 insertions(+), 50 deletions(-) diff --git a/store/store_download.go b/store/store_download.go index 063cc38673f..0b35454331b 100644 --- a/store/store_download.go +++ b/store/store_download.go @@ -320,51 +320,28 @@ func DownloadIcon(ctx context.Context, name string, targetPath string, downloadU // Download icon to a temporary file location, since there may be an active // snap icon hard-linked to targetPath. If download fails, don't want to - // corrupt the existing icon file contents. Instead, move partialPath to - // targetPath once the download succeeds, thus leaving any previous icon - // linked to the original unchanged icon contents, until a future task - // re-links to the new contents. - partialPath := targetPath + ".partial" - - // Open the new partial file and truncate it, since any existing file may - // be old, from a previous version of the snap icon. Don't use `resume` as - // in `Download()`; always start over. - w, err := os.OpenFile(partialPath, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0o600) + // corrupt the existing icon file contents. Instead, commit the new file + // once the download succeeds, thus leaving any previous icon linked to the + // original unchanged icon contents, until a future task re-links to the + // new contents. + aw, err := osutil.NewAtomicFile(targetPath, 0o644, 0, osutil.NoChown, osutil.NoChown) if err != nil { - return err + return fmt.Errorf("cannot create file for snap icon for snap %s: %v", name, err) } - defer func() { - // Unconditionally close the file, whether it's been closed before or - // not. If it has been closed before, close will return an error, which - // we ignore. If it hasn't been called before, then an error has already - // occurred, and we don't care about any new error from calling Close(). - w.Close() - // Unconditionally remove the .partial file, since w.Name() is the name - // with which the file was originally opened. If it still exists, then - // an error occurred, and we want to remove it. If it doesn't still - // exist, then it was successfully renamed, so it's harmless to attempt - // to remove the original .partial filename, and we ignore the error. - os.Remove(w.Name()) - }() - logger.Debugf("Starting download of %q.", partialPath) + // on success, Cancel becomes a no-op + defer aw.Cancel() - err = downloadIcon(ctx, name, downloadURL, w) - if err != nil { - logger.Debugf("download of %q failed: %#v", downloadURL, err) - return err - } + logger.Debugf("Starting download of %q to %q.", downloadURL, targetPath) - if err = w.Sync(); err != nil { + if err = downloadIcon(ctx, name, downloadURL, aw); err != nil { + logger.Debugf("download of %q failed: %#v", downloadURL, err) return err } - if err = w.Close(); err != nil { - return err + if err = aw.Commit(); err != nil { + return fmt.Errorf("cannot commit snap icon file for snap %s: %v", name, err) } - - // Only rename once we know all is well, so we don't want to have any side - // effects on any existing snap icon if an error occurs. - return os.Rename(w.Name(), targetPath) + return nil } func downloadReqOpts(storeURL *url.URL, cdnHeader string, opts *DownloadOptions) *requestOptions { @@ -722,9 +699,10 @@ func downloadIconImpl(ctx context.Context, name, downloadURL string, w io.ReadWr if finalErr != nil { if httputil.ShouldRetryAttempt(attempt, finalErr) { - // XXX: is this correct, without resume? ShouldRetryAttempt - // will return true on io.EOF errors. - // And if so, should we seek w back to 0 in the writer? + if _, err := w.Seek(0, 0); err != nil { + return err + } + // XXX: need to truncate as well continue } break diff --git a/store/store_download_test.go b/store/store_download_test.go index dc6d7af6884..769fa5c5448 100644 --- a/store/store_download_test.go +++ b/store/store_download_test.go @@ -1309,11 +1309,11 @@ func (s *storeDownloadSuite) TestDownloadIconFails(c *C) { fakeName := "foo" fakeURL := "URL" - var tmpfile *os.File + var tmpfile *osutil.AtomicFile restore := store.MockDownloadIcon(func(ctx context.Context, name, url string, w io.ReadWriteSeeker) error { c.Assert(name, Equals, fakeName) c.Assert(url, Equals, fakeURL) - tmpfile = w.(*os.File) + tmpfile = w.(*osutil.AtomicFile) return fmt.Errorf("uh, it failed") }) defer restore() @@ -1332,11 +1332,11 @@ func (s *storeDownloadSuite) TestDownloadIconFailsDoesNotLeavePartial(c *C) { fakeName := "foo" fakeURL := "URL" - var tmpfile *os.File + var tmpfile *osutil.AtomicFile restore := store.MockDownloadIcon(func(ctx context.Context, name, url string, w io.ReadWriteSeeker) error { c.Assert(name, Equals, fakeName) c.Assert(url, Equals, fakeURL) - tmpfile = w.(*os.File) + tmpfile = w.(*osutil.AtomicFile) w.Write([]byte{'X'}) // so it's not empty return fmt.Errorf("uh, it failed") }) @@ -1352,7 +1352,7 @@ func (s *storeDownloadSuite) TestDownloadIconFailsDoesNotLeavePartial(c *C) { c.Assert(osutil.FileExists(path), Equals, false) } -func (s *storeDownloadSuite) TestDownloadIconSyncFailsWithExisting(c *C) { +func (s *storeDownloadSuite) TestDownloadIconFailsWithExisting(c *C) { fakeName := "foo" fakePath := filepath.Join(c.MkDir(), "downloaded-file") fakeURL := "URL" @@ -1370,7 +1370,7 @@ func (s *storeDownloadSuite) TestDownloadIconSyncFailsWithExisting(c *C) { c.Assert(fakePath, testutil.FileEquals, oldContent) } -func (s *storeDownloadSuite) TestDownloadIconSyncFailsWithoutExisting(c *C) { +func (s *storeDownloadSuite) TestDownloadIconFailsWithoutExisting(c *C) { fakeName := "foo" fakePath := filepath.Join(c.MkDir(), "downloaded-file") fakeURL := "URL" @@ -1382,12 +1382,12 @@ func (s *storeDownloadSuite) TestDownloadIconSyncFailsWithoutExisting(c *C) { } func (s *storeDownloadSuite) testDownloadIconSyncFailsGeneric(c *C, fakeName, fakePath, fakeURL string) { - var tmpfile *os.File + var tmpfile *osutil.AtomicFile restore := store.MockDownloadIcon(func(ctx context.Context, name, url string, w io.ReadWriteSeeker) error { c.Assert(name, Equals, fakeName) c.Assert(url, Equals, fakeURL) - tmpfile = w.(*os.File) - w.Write([]byte("sync will fail")) + tmpfile = w.(*osutil.AtomicFile) + w.Write([]byte("commit will fail")) err := tmpfile.Close() c.Assert(err, IsNil) return nil @@ -1396,7 +1396,7 @@ func (s *storeDownloadSuite) testDownloadIconSyncFailsGeneric(c *C, fakeName, fa // simulate a failed sync err := store.DownloadIcon(s.ctx, fakeName, fakePath, fakeURL) - c.Assert(err, ErrorMatches, "(sync|fsync:) .*") + c.Assert(err, ErrorMatches, "cannot commit snap icon file for snap foo: .* file already closed") // ... and ensure that the tempfile is removed c.Assert(osutil.FileExists(tmpfile.Name()), Equals, false) } From 6c6b08fc66e19bdd2c20931d514ef9a9542db4bc Mon Sep 17 00:00:00 2001 From: Oliver Calder Date: Tue, 4 Feb 2025 13:43:21 -0600 Subject: [PATCH 08/13] store: improve downloadIcon following review Signed-off-by: Oliver Calder Co-authored-by: Maciej Borzecki --- store/store_download.go | 108 +++++++++++++++++++++------------------- 1 file changed, 58 insertions(+), 50 deletions(-) diff --git a/store/store_download.go b/store/store_download.go index 0b35454331b..858ae93fd3d 100644 --- a/store/store_download.go +++ b/store/store_download.go @@ -646,73 +646,81 @@ func downloadIconImpl(ctx context.Context, name, downloadURL string, w io.ReadWr return err } - var finalErr error + const maxIconFilesize int64 = 300000 + startTime := time.Now() + errRetry := errors.New("retry") for attempt := retry.Start(downloadRetryStrategy, nil); attempt.Next(); { httputil.MaybeLogRetryAttempt(iconURL.String(), attempt, startTime) - if cancelled(ctx) { - return fmt.Errorf("the download has been cancelled: %s", ctx.Err()) - } - - clientOpts := &httputil.ClientOptions{} // don't need any options - cli := httputil.NewHTTPClient(clientOpts) - var resp *http.Response - resp, finalErr = doIconRequest(ctx, cli, iconURL) - if cancelled(ctx) { - return fmt.Errorf("the download has been cancelled: %s", ctx.Err()) - } - if finalErr != nil { - if httputil.ShouldRetryAttempt(attempt, finalErr) { - continue + err = func() error { + if cancelled(ctx) { + return fmt.Errorf("the download has been cancelled: %s", ctx.Err()) } - break - } - if httputil.ShouldRetryHttpResponse(attempt, resp) { - resp.Body.Close() - continue - } + clientOpts := &httputil.ClientOptions{} // don't need any options + cli := httputil.NewHTTPClient(clientOpts) + var resp *http.Response + resp, err = doIconRequest(ctx, cli, iconURL) + if cancelled(ctx) { + return fmt.Errorf("the download has been cancelled: %s", ctx.Err()) + } + if err != nil { + if httputil.ShouldRetryAttempt(attempt, err) { + return errRetry + } + return err + } - // XXX: we're inside retry loop, so this will be closed only on return. - defer resp.Body.Close() + defer resp.Body.Close() - switch resp.StatusCode { - case 200: // OK - default: - return &DownloadError{Code: resp.StatusCode, URL: resp.Request.URL} - } + if httputil.ShouldRetryHttpResponse(attempt, resp) { + return errRetry + } - const maxIconFilesize int64 = 300000 + switch resp.StatusCode { + case 200: // OK + default: + return &DownloadError{Code: resp.StatusCode, URL: resp.Request.URL} + } - if resp.ContentLength == 0 || resp.ContentLength > maxIconFilesize { - return fmt.Errorf("unsupported Content-Length for %s (must be nonzero and <%dB): %d", downloadURL, maxIconFilesize, resp.ContentLength) - } else { - logger.Debugf("Download size for %s: %d", downloadURL, resp.ContentLength) - } + if resp.ContentLength == 0 || resp.ContentLength > maxIconFilesize { + return fmt.Errorf("unsupported Content-Length for %s (must be nonzero and <%dB): %d", downloadURL, maxIconFilesize, resp.ContentLength) + } else { + logger.Debugf("download size for %s: %d", downloadURL, resp.ContentLength) + } - _, finalErr = io.CopyN(w, resp.Body, resp.ContentLength) + _, err = io.CopyN(w, resp.Body, resp.ContentLength) - if cancelled(ctx) { - return fmt.Errorf("the download has been cancelled: %s", ctx.Err()) - } + if cancelled(ctx) { + return fmt.Errorf("the download has been cancelled: %s", ctx.Err()) + } - if finalErr != nil { - if httputil.ShouldRetryAttempt(attempt, finalErr) { - if _, err := w.Seek(0, 0); err != nil { - return err + if err != nil { + if httputil.ShouldRetryAttempt(attempt, err) { + if _, err := w.Seek(0, 0); err != nil { + return err + } + // XXX: need to truncate as well + return errRetry } - // XXX: need to truncate as well - continue + return err } - break + + return nil + }() + + if err == errRetry { + continue } - break - } - if finalErr == nil { - logger.Debugf("Icon download succeeded") + + if err == nil { + logger.Debugf("icon download succeeded for %s", name) + } + + return err } - return finalErr + return nil } // DownloadStream will copy the snap from the request to the io.Reader From 0c5ba5984a69c173d2ce337b3a232e1071c4c4af Mon Sep 17 00:00:00 2001 From: Oliver Calder Date: Tue, 4 Feb 2025 14:00:33 -0600 Subject: [PATCH 09/13] store: truncate icon file when retrying download Signed-off-by: Oliver Calder --- store/download_test.go | 11 +++++++++-- store/export_test.go | 2 +- store/store_download.go | 14 ++++++++++++-- store/store_download_test.go | 10 +++++----- 4 files changed, 27 insertions(+), 10 deletions(-) diff --git a/store/download_test.go b/store/download_test.go index 0f739dd15bb..75189ee32a7 100644 --- a/store/download_test.go +++ b/store/download_test.go @@ -291,8 +291,8 @@ func (s *downloadSuite) TestActualDownload500Once(c *C) { c.Check(n, Equals, 2) } -// SillyBuffer is a ReadWriteSeeker buffer with a limited size for the tests -// (bytes does not implement an ReadWriteSeeker) +// SillyBuffer is a ReadWriteSeekTruncater buffer with a limited size for the tests +// (bytes does not implement an ReadWriteSeekTruncater) type SillyBuffer struct { buf [1024]byte pos int64 @@ -333,6 +333,13 @@ func (sb *SillyBuffer) Write(p []byte) (n int, err error) { } return n, nil } +func (sb *SillyBuffer) Truncate(size int64) error { + if size < 0 || size > int64(len(sb.buf)) { + return fmt.Errorf("truncate out of bounds: %d", size) + } + sb.end = size + return nil +} func (sb *SillyBuffer) String() string { return string(sb.buf[0:sb.pos]) } diff --git a/store/export_test.go b/store/export_test.go index e279441ffc5..9ec5f57cc05 100644 --- a/store/export_test.go +++ b/store/export_test.go @@ -151,7 +151,7 @@ func MockDownload(f func(ctx context.Context, name, sha3_384, downloadURL string } } -func MockDownloadIcon(f func(ctx context.Context, name, downloadURL string, w io.ReadWriteSeeker) error) (restore func()) { +func MockDownloadIcon(f func(ctx context.Context, name, downloadURL string, w ReadWriteSeekTruncater) error) (restore func()) { return testutil.Mock(&downloadIcon, f) } diff --git a/store/store_download.go b/store/store_download.go index 858ae93fd3d..1a662f75e43 100644 --- a/store/store_download.go +++ b/store/store_download.go @@ -636,11 +636,19 @@ func downloadImpl(ctx context.Context, name, sha3_384, downloadURL string, user return finalErr } +type ReadWriteSeekTruncater interface { + io.Reader + io.Writer + io.Seeker + + Truncate(size int64) error +} + var downloadIcon = downloadIconImpl // downloadIconImpl writes an http.Request which does not require authentication // or a progress.Meter. -func downloadIconImpl(ctx context.Context, name, downloadURL string, w io.ReadWriteSeeker) error { +func downloadIconImpl(ctx context.Context, name, downloadURL string, w ReadWriteSeekTruncater) error { iconURL, err := url.Parse(downloadURL) if err != nil { return err @@ -701,7 +709,9 @@ func downloadIconImpl(ctx context.Context, name, downloadURL string, w io.ReadWr if _, err := w.Seek(0, 0); err != nil { return err } - // XXX: need to truncate as well + if err := w.Truncate(0); err != nil { + return err + } return errRetry } return err diff --git a/store/store_download_test.go b/store/store_download_test.go index 769fa5c5448..9626ae4301f 100644 --- a/store/store_download_test.go +++ b/store/store_download_test.go @@ -1257,7 +1257,7 @@ func (s *storeDownloadSuite) TestDownloadIconOK(c *C) { expectedURL := "URL" expectedContent := []byte("I was downloaded") - restore := store.MockDownloadIcon(func(ctx context.Context, name, url string, w io.ReadWriteSeeker) error { + restore := store.MockDownloadIcon(func(ctx context.Context, name, url string, w store.ReadWriteSeekTruncater) error { c.Check(url, Equals, expectedURL) w.Write(expectedContent) return nil @@ -1277,7 +1277,7 @@ func (s *storeDownloadSuite) TestDownloadIconDoesNotOverwriteLinks(c *C) { oldContent := []byte("I was already here") newContent := []byte("I was downloaded") - restore := store.MockDownloadIcon(func(ctx context.Context, name, url string, w io.ReadWriteSeeker) error { + restore := store.MockDownloadIcon(func(ctx context.Context, name, url string, w store.ReadWriteSeekTruncater) error { c.Check(url, Equals, expectedURL) w.Write(newContent) return nil @@ -1310,7 +1310,7 @@ func (s *storeDownloadSuite) TestDownloadIconFails(c *C) { fakeURL := "URL" var tmpfile *osutil.AtomicFile - restore := store.MockDownloadIcon(func(ctx context.Context, name, url string, w io.ReadWriteSeeker) error { + restore := store.MockDownloadIcon(func(ctx context.Context, name, url string, w store.ReadWriteSeekTruncater) error { c.Assert(name, Equals, fakeName) c.Assert(url, Equals, fakeURL) tmpfile = w.(*osutil.AtomicFile) @@ -1333,7 +1333,7 @@ func (s *storeDownloadSuite) TestDownloadIconFailsDoesNotLeavePartial(c *C) { fakeURL := "URL" var tmpfile *osutil.AtomicFile - restore := store.MockDownloadIcon(func(ctx context.Context, name, url string, w io.ReadWriteSeeker) error { + restore := store.MockDownloadIcon(func(ctx context.Context, name, url string, w store.ReadWriteSeekTruncater) error { c.Assert(name, Equals, fakeName) c.Assert(url, Equals, fakeURL) tmpfile = w.(*osutil.AtomicFile) @@ -1383,7 +1383,7 @@ func (s *storeDownloadSuite) TestDownloadIconFailsWithoutExisting(c *C) { func (s *storeDownloadSuite) testDownloadIconSyncFailsGeneric(c *C, fakeName, fakePath, fakeURL string) { var tmpfile *osutil.AtomicFile - restore := store.MockDownloadIcon(func(ctx context.Context, name, url string, w io.ReadWriteSeeker) error { + restore := store.MockDownloadIcon(func(ctx context.Context, name, url string, w store.ReadWriteSeekTruncater) error { c.Assert(name, Equals, fakeName) c.Assert(url, Equals, fakeURL) tmpfile = w.(*osutil.AtomicFile) From dfaa1bb5aacbf43955245c68319dca24e529cd87 Mon Sep 17 00:00:00 2001 From: Oliver Calder Date: Tue, 4 Feb 2025 14:29:31 -0600 Subject: [PATCH 10/13] store: add test that max icon filesize is obeyed Signed-off-by: Oliver Calder --- store/download_test.go | 20 ++++++++++++++++++++ store/export_test.go | 4 ++++ store/store_download.go | 7 ++++--- 3 files changed, 28 insertions(+), 3 deletions(-) diff --git a/store/download_test.go b/store/download_test.go index 75189ee32a7..b5fc74c0716 100644 --- a/store/download_test.go +++ b/store/download_test.go @@ -735,6 +735,26 @@ func (s *downloadSuite) TestActualDownloadIcon(c *C) { c.Check(n, Equals, 1) } +func (s *downloadSuite) TestActualDownloadIconTooLarge(c *C) { + var maxSize int64 = 1000 // Must be less than size of SillyBuffer, so we can write enough to exceed the limit + restore := store.MockMaxIconFilesize(maxSize) + defer restore() + + n := 0 + mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + n++ + response := make([]byte, maxSize+1) + w.Write(response) + })) + c.Assert(mockServer, NotNil) + defer mockServer.Close() + + var buf SillyBuffer + err := store.DownloadIconImpl(context.TODO(), "foo", mockServer.URL, &buf) + c.Assert(err, ErrorMatches, "unsupported Content-Length .*") + c.Check(n, Equals, 1) +} + func (s *downloadSuite) TestDownloadIconCancellation(c *C) { // the channel used by mock server to request cancellation from the test syncCh := make(chan struct{}) diff --git a/store/export_test.go b/store/export_test.go index 9ec5f57cc05..2d027fe8777 100644 --- a/store/export_test.go +++ b/store/export_test.go @@ -151,6 +151,10 @@ func MockDownload(f func(ctx context.Context, name, sha3_384, downloadURL string } } +func MockMaxIconFilesize(maxSize int64) (restore func()) { + return testutil.Mock(&maxIconFilesize, maxSize) +} + func MockDownloadIcon(f func(ctx context.Context, name, downloadURL string, w ReadWriteSeekTruncater) error) (restore func()) { return testutil.Mock(&downloadIcon, f) } diff --git a/store/store_download.go b/store/store_download.go index 1a662f75e43..8a5f6b56bfd 100644 --- a/store/store_download.go +++ b/store/store_download.go @@ -644,7 +644,10 @@ type ReadWriteSeekTruncater interface { Truncate(size int64) error } -var downloadIcon = downloadIconImpl +var ( + maxIconFilesize int64 = 300000 + downloadIcon = downloadIconImpl +) // downloadIconImpl writes an http.Request which does not require authentication // or a progress.Meter. @@ -654,8 +657,6 @@ func downloadIconImpl(ctx context.Context, name, downloadURL string, w ReadWrite return err } - const maxIconFilesize int64 = 300000 - startTime := time.Now() errRetry := errors.New("retry") for attempt := retry.Start(downloadRetryStrategy, nil); attempt.Next(); { From 97ce6c4f53463eb7936f049f8b9ebbd445e145ff Mon Sep 17 00:00:00 2001 From: Oliver Calder Date: Tue, 4 Feb 2025 16:22:12 -0600 Subject: [PATCH 11/13] store: remove explicit cancellation checks and simplify tests Also add comments noting that the http clients for both snap downloads and snap icon downloads have no timeout defined. Signed-off-by: Oliver Calder --- store/download_test.go | 46 ++++++++++------------------------------- store/store_download.go | 18 +++------------- 2 files changed, 14 insertions(+), 50 deletions(-) diff --git a/store/download_test.go b/store/download_test.go index b5fc74c0716..ae5bbbcb4e7 100644 --- a/store/download_test.go +++ b/store/download_test.go @@ -172,14 +172,13 @@ func (s *downloadSuite) TestActualDownloadLessDetailedCloudInfoFromAuthContext(c } func (s *downloadSuite) TestDownloadCancellation(c *C) { - // the channel used by mock server to request cancellation from the test - syncCh := make(chan struct{}) + ctx, cancel := context.WithCancel(context.Background()) n := 0 mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { n++ io.WriteString(w, "foo") - syncCh <- struct{}{} + cancel() io.WriteString(w, "bar") time.Sleep(10 * time.Millisecond) })) @@ -188,23 +187,12 @@ func (s *downloadSuite) TestDownloadCancellation(c *C) { theStore := store.New(&store.Config{}, nil) - ctx, cancel := context.WithCancel(context.Background()) - - result := make(chan string) - go func() { - sha3 := "" - var buf SillyBuffer - err := store.Download(ctx, "foo", sha3, mockServer.URL, nil, theStore, &buf, 0, nil, nil) - result <- err.Error() - close(result) - }() - - <-syncCh - cancel() + sha3 := "" + var buf SillyBuffer + err := store.Download(ctx, "foo", sha3, mockServer.URL, nil, theStore, &buf, 0, nil, nil) - err := <-result c.Check(n, Equals, 1) - c.Assert(err, Equals, "the download has been cancelled: context canceled") + c.Assert(err, ErrorMatches, "the download has been cancelled: context canceled") } type nopeSeeker struct{ io.ReadWriter } @@ -756,36 +744,24 @@ func (s *downloadSuite) TestActualDownloadIconTooLarge(c *C) { } func (s *downloadSuite) TestDownloadIconCancellation(c *C) { - // the channel used by mock server to request cancellation from the test - syncCh := make(chan struct{}) + ctx, cancel := context.WithCancel(context.Background()) n := 0 mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { n++ io.WriteString(w, "foo") - syncCh <- struct{}{} + cancel() io.WriteString(w, "bar") time.Sleep(10 * time.Millisecond) })) c.Assert(mockServer, NotNil) defer mockServer.Close() - ctx, cancel := context.WithCancel(context.Background()) - - result := make(chan string) - go func() { - var buf SillyBuffer - err := store.DownloadIconImpl(ctx, "foo", mockServer.URL, &buf) - result <- err.Error() - close(result) - }() - - <-syncCh - cancel() + var buf SillyBuffer + err := store.DownloadIconImpl(ctx, "foo", mockServer.URL, &buf) - err := <-result c.Check(n, Equals, 1) - c.Assert(err, Equals, "the download has been cancelled: context canceled") + c.Assert(err, ErrorMatches, ".*: context canceled") } func (s *downloadSuite) TestActualDownloadIcon404(c *C) { diff --git a/store/store_download.go b/store/store_download.go index 8a5f6b56bfd..19122b26499 100644 --- a/store/store_download.go +++ b/store/store_download.go @@ -521,7 +521,7 @@ func downloadImpl(ctx context.Context, name, sha3_384, downloadURL string, user return fmt.Errorf("the download has been cancelled: %s", downloadCtx.Err()) } var resp *http.Response - cli := s.newHTTPClient(nil) + cli := s.newHTTPClient(nil) // XXX: there's no timeout defined for this client, and the context is context.TODO(), so it won't be cancelled oldCheckRedirect := cli.CheckRedirect if oldCheckRedirect == nil { panic("internal error: the httputil.NewHTTPClient-produced http.Client must have CheckRedirect defined") @@ -663,17 +663,10 @@ func downloadIconImpl(ctx context.Context, name, downloadURL string, w ReadWrite httputil.MaybeLogRetryAttempt(iconURL.String(), attempt, startTime) err = func() error { - if cancelled(ctx) { - return fmt.Errorf("the download has been cancelled: %s", ctx.Err()) - } - - clientOpts := &httputil.ClientOptions{} // don't need any options + clientOpts := &httputil.ClientOptions{} // XXX: should this have a timeout? cli := httputil.NewHTTPClient(clientOpts) var resp *http.Response resp, err = doIconRequest(ctx, cli, iconURL) - if cancelled(ctx) { - return fmt.Errorf("the download has been cancelled: %s", ctx.Err()) - } if err != nil { if httputil.ShouldRetryAttempt(attempt, err) { return errRetry @@ -695,16 +688,11 @@ func downloadIconImpl(ctx context.Context, name, downloadURL string, w ReadWrite if resp.ContentLength == 0 || resp.ContentLength > maxIconFilesize { return fmt.Errorf("unsupported Content-Length for %s (must be nonzero and <%dB): %d", downloadURL, maxIconFilesize, resp.ContentLength) - } else { - logger.Debugf("download size for %s: %d", downloadURL, resp.ContentLength) } + logger.Debugf("download size for %s: %d", downloadURL, resp.ContentLength) _, err = io.CopyN(w, resp.Body, resp.ContentLength) - if cancelled(ctx) { - return fmt.Errorf("the download has been cancelled: %s", ctx.Err()) - } - if err != nil { if httputil.ShouldRetryAttempt(attempt, err) { if _, err := w.Seek(0, 0); err != nil { From 462c66948aee48927c2d6560165c7bb0244d4a1e Mon Sep 17 00:00:00 2001 From: Oliver Calder Date: Tue, 4 Feb 2025 16:36:09 -0600 Subject: [PATCH 12/13] store: simplify icon download status code handling Signed-off-by: Oliver Calder --- store/store_download.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/store/store_download.go b/store/store_download.go index 19122b26499..c2f7001f45a 100644 --- a/store/store_download.go +++ b/store/store_download.go @@ -680,9 +680,7 @@ func downloadIconImpl(ctx context.Context, name, downloadURL string, w ReadWrite return errRetry } - switch resp.StatusCode { - case 200: // OK - default: + if resp.StatusCode != 200 { return &DownloadError{Code: resp.StatusCode, URL: resp.Request.URL} } From 215774e09949f671118e2ae157134b5f6c88ea91 Mon Sep 17 00:00:00 2001 From: Oliver Calder Date: Tue, 4 Feb 2025 16:43:40 -0600 Subject: [PATCH 13/13] store: improve consistency of icon download tests Signed-off-by: Oliver Calder --- store/store_download_test.go | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/store/store_download_test.go b/store/store_download_test.go index 9626ae4301f..044c791b8c1 100644 --- a/store/store_download_test.go +++ b/store/store_download_test.go @@ -1306,8 +1306,9 @@ func (s *storeDownloadSuite) TestDownloadIconDoesNotOverwriteLinks(c *C) { } func (s *storeDownloadSuite) TestDownloadIconFails(c *C) { - fakeName := "foo" - fakeURL := "URL" + const fakeName = "foo" + fakePath := filepath.Join(c.MkDir(), "downloaded-file") + const fakeURL = "URL" var tmpfile *osutil.AtomicFile restore := store.MockDownloadIcon(func(ctx context.Context, name, url string, w store.ReadWriteSeekTruncater) error { @@ -1319,18 +1320,18 @@ func (s *storeDownloadSuite) TestDownloadIconFails(c *C) { defer restore() // simulate a failed download - path := filepath.Join(c.MkDir(), "downloaded-file") - err := store.DownloadIcon(s.ctx, fakeName, path, fakeURL) + err := store.DownloadIcon(s.ctx, fakeName, fakePath, fakeURL) c.Assert(err, ErrorMatches, "uh, it failed") // ... and ensure that the tempfile is removed c.Assert(osutil.FileExists(tmpfile.Name()), Equals, false) // ... and not because it succeeded either - c.Assert(osutil.FileExists(path), Equals, false) + c.Assert(osutil.FileExists(fakePath), Equals, false) } func (s *storeDownloadSuite) TestDownloadIconFailsDoesNotLeavePartial(c *C) { - fakeName := "foo" - fakeURL := "URL" + const fakeName = "foo" + fakePath := filepath.Join(c.MkDir(), "downloaded-file") + const fakeURL = "URL" var tmpfile *osutil.AtomicFile restore := store.MockDownloadIcon(func(ctx context.Context, name, url string, w store.ReadWriteSeekTruncater) error { @@ -1343,19 +1344,18 @@ func (s *storeDownloadSuite) TestDownloadIconFailsDoesNotLeavePartial(c *C) { defer restore() // simulate a failed download - path := filepath.Join(c.MkDir(), "downloaded-file") - err := store.DownloadIcon(s.ctx, fakeName, path, fakeURL) + err := store.DownloadIcon(s.ctx, fakeName, fakePath, fakeURL) c.Assert(err, ErrorMatches, "uh, it failed") // ... and ensure that the tempfile is removed c.Assert(osutil.FileExists(tmpfile.Name()), Equals, false) // ... and the target path isn't there - c.Assert(osutil.FileExists(path), Equals, false) + c.Assert(osutil.FileExists(fakePath), Equals, false) } func (s *storeDownloadSuite) TestDownloadIconFailsWithExisting(c *C) { - fakeName := "foo" + const fakeName = "foo" fakePath := filepath.Join(c.MkDir(), "downloaded-file") - fakeURL := "URL" + const fakeURL = "URL" // Create an existing file at the path oldContent := []byte("I was already here") @@ -1371,9 +1371,9 @@ func (s *storeDownloadSuite) TestDownloadIconFailsWithExisting(c *C) { } func (s *storeDownloadSuite) TestDownloadIconFailsWithoutExisting(c *C) { - fakeName := "foo" + const fakeName = "foo" fakePath := filepath.Join(c.MkDir(), "downloaded-file") - fakeURL := "URL" + const fakeURL = "URL" s.testDownloadIconSyncFailsGeneric(c, fakeName, fakePath, fakeURL) @@ -1416,10 +1416,10 @@ func (s *storeDownloadSuite) TestDownloadIconInfiniteRedirect(c *C) { c.Assert(mockServer, NotNil) defer mockServer.Close() - fakeName := "foo" + const fakeName = "foo" + fakePath := filepath.Join(c.MkDir(), "foo.icon") fakeURL := mockServer.URL - targetPath := filepath.Join(c.MkDir(), "foo.icon") - err := store.DownloadIcon(s.ctx, fakeName, targetPath, fakeURL) + err := store.DownloadIcon(s.ctx, fakeName, fakePath, fakeURL) c.Assert(err, ErrorMatches, fmt.Sprintf("Get %q: stopped after 10 redirects", fakeURL)) }