From c46dd3f138f3b82c8ee2749a62744bf20c97a3f5 Mon Sep 17 00:00:00 2001 From: Erik Seliger Date: Mon, 15 Aug 2022 23:10:30 +0200 Subject: [PATCH 1/4] batches: Make sure no corrupted repozips are used When the download of a zip was aborted, it could've left behind a corrupted archive zip. This fixes it by first storing it to a temporary location and then creating the actual file atomically by doing os.Rename. --- internal/batches/repozip/fetcher.go | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/internal/batches/repozip/fetcher.go b/internal/batches/repozip/fetcher.go index dff57bad00..9a566308a4 100644 --- a/internal/batches/repozip/fetcher.go +++ b/internal/batches/repozip/fetcher.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "io" + "math/rand" "net/http" "os" "path" @@ -336,7 +337,10 @@ func fetchRepositoryFile(ctx context.Context, client HTTPClient, repo RepoRevisi return false, fmt.Errorf("unable to fetch archive (HTTP %d from %s)", resp.StatusCode, req.URL.String()) } - f, err := os.Create(dest) + tmpFileDir := fmt.Sprintf("%s.%d.tmp", dest, rand.Int()) + // Ensure the file doesn't exist. + _ = os.Remove(tmpFileDir) + f, err := os.Create(tmpFileDir) if err != nil { return false, err } @@ -346,6 +350,12 @@ func fetchRepositoryFile(ctx context.Context, client HTTPClient, repo RepoRevisi return false, err } + // Atomically create the actual file, so that there are no artifacts left behind + // when this process is aborted, network errors occur, or some witchcraft goes on. + if err := os.Rename(tmpFileDir, dest); err != nil { + return false, errors.Wrap(err, "renaming temp file") + } + return true, nil } From 8c04c68a196fe7b049cb6ff08406ed7378b92bff Mon Sep 17 00:00:00 2001 From: Erik Seliger Date: Mon, 15 Aug 2022 23:24:53 +0200 Subject: [PATCH 2/4] Use os.CreateTemp --- internal/batches/repozip/fetcher.go | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/internal/batches/repozip/fetcher.go b/internal/batches/repozip/fetcher.go index 9a566308a4..46c89323b4 100644 --- a/internal/batches/repozip/fetcher.go +++ b/internal/batches/repozip/fetcher.go @@ -4,7 +4,6 @@ import ( "context" "fmt" "io" - "math/rand" "net/http" "os" "path" @@ -337,10 +336,7 @@ func fetchRepositoryFile(ctx context.Context, client HTTPClient, repo RepoRevisi return false, fmt.Errorf("unable to fetch archive (HTTP %d from %s)", resp.StatusCode, req.URL.String()) } - tmpFileDir := fmt.Sprintf("%s.%d.tmp", dest, rand.Int()) - // Ensure the file doesn't exist. - _ = os.Remove(tmpFileDir) - f, err := os.Create(tmpFileDir) + f, err := os.CreateTemp(path.Dir(dest), fmt.Sprintf("%s-*.tmp", path.Base(dest))) if err != nil { return false, err } @@ -352,7 +348,7 @@ func fetchRepositoryFile(ctx context.Context, client HTTPClient, repo RepoRevisi // Atomically create the actual file, so that there are no artifacts left behind // when this process is aborted, network errors occur, or some witchcraft goes on. - if err := os.Rename(tmpFileDir, dest); err != nil { + if err := os.Rename(f.Name(), dest); err != nil { return false, errors.Wrap(err, "renaming temp file") } From e65e2289a662006db0f9d865a86ffc35a71abd37 Mon Sep 17 00:00:00 2001 From: Erik Seliger Date: Mon, 15 Aug 2022 23:30:11 +0200 Subject: [PATCH 3/4] More safe? --- internal/batches/repozip/fetcher.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/internal/batches/repozip/fetcher.go b/internal/batches/repozip/fetcher.go index 46c89323b4..14f9edf8d1 100644 --- a/internal/batches/repozip/fetcher.go +++ b/internal/batches/repozip/fetcher.go @@ -340,11 +340,17 @@ func fetchRepositoryFile(ctx context.Context, client HTTPClient, repo RepoRevisi if err != nil { return false, err } - defer f.Close() + // Make sure we clean up the temp file in case something fails. + defer func(path string) { _ = os.Remove(path) }(f.Name()) if _, err := io.Copy(f, resp.Body); err != nil { + // Be a good citizen, attempt to close the file. + _ = f.Close() return false, err } + if err := f.Close(); err != nil { + return false, errors.Wrap(err, "closing temp file") + } // Atomically create the actual file, so that there are no artifacts left behind // when this process is aborted, network errors occur, or some witchcraft goes on. From 7ca6e7362d9892aa10a0d4fef7ed0119aebb2265 Mon Sep 17 00:00:00 2001 From: Erik Seliger Date: Mon, 15 Aug 2022 23:42:58 +0200 Subject: [PATCH 4/4] Maybe fix on windows? --- internal/batches/repozip/fetcher.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/batches/repozip/fetcher.go b/internal/batches/repozip/fetcher.go index 14f9edf8d1..8146bcf07b 100644 --- a/internal/batches/repozip/fetcher.go +++ b/internal/batches/repozip/fetcher.go @@ -336,7 +336,7 @@ func fetchRepositoryFile(ctx context.Context, client HTTPClient, repo RepoRevisi return false, fmt.Errorf("unable to fetch archive (HTTP %d from %s)", resp.StatusCode, req.URL.String()) } - f, err := os.CreateTemp(path.Dir(dest), fmt.Sprintf("%s-*.tmp", path.Base(dest))) + f, err := os.CreateTemp(filepath.Dir(dest), fmt.Sprintf("%s-*.tmp", filepath.Base(dest))) if err != nil { return false, err }