diff --git a/oci/archive/oci_dest.go b/oci/archive/oci_dest.go index 490eefe9..5aa40161 100644 --- a/oci/archive/oci_dest.go +++ b/oci/archive/oci_dest.go @@ -178,7 +178,7 @@ func (d *ociArchiveImageDestination) CommitWithOptions(ctx context.Context, opti // tar converts the directory at src and saves it to dst // if contentModTimes is non-nil, tar header entries times are set to this -func tarDirectory(src, dst string, contentModTimes *time.Time) error { +func tarDirectory(src, dst string, contentModTimes *time.Time) (retErr error) { // input is a stream of bytes from the archive of the directory at path input, err := archive.TarWithOptions(src, &archive.TarOptions{ Compression: archive.Uncompressed, @@ -197,7 +197,14 @@ func tarDirectory(src, dst string, contentModTimes *time.Time) error { if err != nil { return fmt.Errorf("creating tar file %q: %w", dst, err) } - defer outFile.Close() + + // since we are writing to this file, make sure we handle errors + defer func() { + closeErr := outFile.Close() + if retErr == nil { + retErr = closeErr + } + }() // copies the contents of the directory to the tar file // TODO: This can take quite some time, and should ideally be cancellable using a context.Context. diff --git a/oci/layout/oci_delete.go b/oci/layout/oci_delete.go index 08366a7e..f32dd53d 100644 --- a/oci/layout/oci_delete.go +++ b/oci/layout/oci_delete.go @@ -159,7 +159,7 @@ func (ref ociReference) deleteReferenceFromIndex(referenceIndex int) error { return saveJSON(ref.indexPath(), index) } -func saveJSON(path string, content any) error { +func saveJSON(path string, content any) (retErr error) { // If the file already exists, get its mode to preserve it var mode fs.FileMode existingfi, err := os.Stat(path) @@ -177,7 +177,13 @@ func saveJSON(path string, content any) error { if err != nil { return err } - defer file.Close() + // since we are writing to this file, make sure we handle errors + defer func() { + closeErr := file.Close() + if retErr == nil { + retErr = closeErr + } + }() return json.NewEncoder(file).Encode(content) } diff --git a/oci/layout/oci_dest.go b/oci/layout/oci_dest.go index 2ed6b91b..492ede59 100644 --- a/oci/layout/oci_dest.go +++ b/oci/layout/oci_dest.go @@ -115,7 +115,7 @@ func (d *ociImageDestination) Close() error { // WARNING: The contents of stream are being verified on the fly. Until stream.Read() returns io.EOF, the contents of the data SHOULD NOT be available // to any other readers for download using the supplied digest. // If stream.Read() at any time, ESPECIALLY at end of input, returns an error, PutBlobWithOptions MUST 1) fail, and 2) delete any data stored so far. -func (d *ociImageDestination) PutBlobWithOptions(ctx context.Context, stream io.Reader, inputInfo types.BlobInfo, options private.PutBlobOptions) (private.UploadedBlob, error) { +func (d *ociImageDestination) PutBlobWithOptions(ctx context.Context, stream io.Reader, inputInfo types.BlobInfo, options private.PutBlobOptions) (_ private.UploadedBlob, retErr error) { blobFile, err := os.CreateTemp(d.ref.dir, "oci-put-blob") if err != nil { return private.UploadedBlob{}, err @@ -124,7 +124,10 @@ func (d *ociImageDestination) PutBlobWithOptions(ctx context.Context, stream io. explicitClosed := false defer func() { if !explicitClosed { - blobFile.Close() + closeErr := blobFile.Close() + if retErr == nil { + retErr = closeErr + } } if !succeeded { os.Remove(blobFile.Name()) @@ -176,7 +179,10 @@ func (d *ociImageDestination) blobFileSyncAndRename(blobFile *os.File, blobDiges } // need to explicitly close the file, since a rename won't otherwise work on Windows - blobFile.Close() + err = blobFile.Close() + if err != nil { + return err + } *closed = true if err := os.Rename(blobFile.Name(), blobPath); err != nil { @@ -323,7 +329,7 @@ type PutBlobFromLocalFileOption struct{} // It computes, and returns, the digest and size of the used file. // // This function can be used instead of dest.PutBlob() where the ImageDestination requires PutBlob() to be called. -func PutBlobFromLocalFile(ctx context.Context, dest types.ImageDestination, file string, options ...PutBlobFromLocalFileOption) (digest.Digest, int64, error) { +func PutBlobFromLocalFile(ctx context.Context, dest types.ImageDestination, file string, options ...PutBlobFromLocalFileOption) (_ digest.Digest, _ int64, retErr error) { d, ok := dest.(*ociImageDestination) if !ok { return "", -1, errors.New("caller error: PutBlobFromLocalFile called with a non-oci: destination") @@ -337,7 +343,10 @@ func PutBlobFromLocalFile(ctx context.Context, dest types.ImageDestination, file } defer func() { if !blobFileClosed { - blobFile.Close() + closeErr := blobFile.Close() + if retErr == nil { + retErr = closeErr + } } if !succeeded { os.Remove(blobFile.Name()) diff --git a/ostree/ostree_dest.go b/ostree/ostree_dest.go index d4ebe413..4703770f 100644 --- a/ostree/ostree_dest.go +++ b/ostree/ostree_dest.go @@ -143,16 +143,24 @@ func (d *ostreeImageDestination) PutBlobWithOptions(ctx context.Context, stream return private.UploadedBlob{}, err } + digester, stream := putblobdigest.DigestIfCanonicalUnknown(stream, inputInfo) + blobPath := filepath.Join(tmpDir, "content") blobFile, err := os.Create(blobPath) if err != nil { return private.UploadedBlob{}, err } - defer blobFile.Close() - - digester, stream := putblobdigest.DigestIfCanonicalUnknown(stream, inputInfo) - // TODO: This can take quite some time, and should ideally be cancellable using ctx.Done(). - size, err := io.Copy(blobFile, stream) + size, err := func() (_ int64, retErr error) { // A scope for defer + // since we are writing to this file, make sure we handle errors + defer func() { + closeErr := blobFile.Close() + if retErr == nil { + retErr = closeErr + } + }() + // TODO: This can take quite some time, and should ideally be cancellable using ctx.Done(). + return io.Copy(blobFile, stream) + }() if err != nil { return private.UploadedBlob{}, err } @@ -247,9 +255,15 @@ func (d *ostreeImageDestination) ostreeCommit(repo *otbuiltin.Repo, branch strin return err } -func generateTarSplitMetadata(output *bytes.Buffer, file string) (digest.Digest, int64, error) { +func generateTarSplitMetadata(output *bytes.Buffer, file string) (_ digest.Digest, _ int64, retErr error) { mfz := pgzip.NewWriter(output) - defer mfz.Close() + // since we are writing to this, make sure we handle errors + defer func() { + closeErr := mfz.Close() + if retErr == nil { + retErr = closeErr + } + }() metaPacker := storage.NewJSONPacker(mfz) stream, err := os.OpenFile(file, os.O_RDONLY, 0) diff --git a/pkg/blobinfocache/sqlite/sqlite.go b/pkg/blobinfocache/sqlite/sqlite.go index 1a793102..719c8eda 100644 --- a/pkg/blobinfocache/sqlite/sqlite.go +++ b/pkg/blobinfocache/sqlite/sqlite.go @@ -87,14 +87,20 @@ func new2(path string) (*cache, error) { if err != nil { return nil, fmt.Errorf("initializing blob info cache at %q: %w", path, err) } - defer db.Close() - - // We don’t check the schema before every operation, because that would be costly - // and because we assume schema changes will be handled by using a different path. - if err := ensureDBHasCurrentSchema(db); err != nil { + err = func() (retErr error) { // A scope for defer + defer func() { + closeErr := db.Close() + if retErr == nil { + retErr = closeErr + } + }() + // We don’t check the schema before every operation, because that would be costly + // and because we assume schema changes will be handled by using a different path. + return ensureDBHasCurrentSchema(db) + }() + if err != nil { return nil, err } - return &cache{ path: path, refCount: 0, @@ -147,25 +153,30 @@ func (sqc *cache) Close() { type void struct{} // So that we don’t have to write struct{}{} all over the place // transaction calls fn within a read-write transaction in sqc. -func transaction[T any](sqc *cache, fn func(tx *sql.Tx) (T, error)) (T, error) { - db, closeDB, err := func() (*sql.DB, func(), error) { // A scope for defer +func transaction[T any](sqc *cache, fn func(tx *sql.Tx) (T, error)) (_ T, retErr error) { + db, closeDB, err := func() (*sql.DB, func() error, error) { // A scope for defer sqc.lock.Lock() defer sqc.lock.Unlock() if sqc.db != nil { - return sqc.db, func() {}, nil + return sqc.db, func() error { return nil }, nil } db, err := rawOpen(sqc.path) if err != nil { return nil, nil, fmt.Errorf("opening blob info cache at %q: %w", sqc.path, err) } - return db, func() { db.Close() }, nil + return db, db.Close, nil }() if err != nil { var zeroRes T // A zero value of T return zeroRes, err } - defer closeDB() + defer func() { + closeErr := closeDB() + if retErr == nil { + retErr = closeErr + } + }() return dbTransaction(db, fn) } diff --git a/pkg/sysregistriesv2/shortnames.go b/pkg/sysregistriesv2/shortnames.go index 71f5bc83..d5280885 100644 --- a/pkg/sysregistriesv2/shortnames.go +++ b/pkg/sysregistriesv2/shortnames.go @@ -134,7 +134,7 @@ func ResolveShortNameAlias(ctx *types.SystemContext, name string) (reference.Nam // editShortNameAlias loads the aliases.conf file and changes it. If value is // set, it adds the name-value pair as a new alias. Otherwise, it will remove // name from the config. -func editShortNameAlias(ctx *types.SystemContext, name string, value *string) error { +func editShortNameAlias(ctx *types.SystemContext, name string, value *string) (retErr error) { if err := validateShortName(name); err != nil { return err } @@ -178,7 +178,13 @@ func editShortNameAlias(ctx *types.SystemContext, name string, value *string) er if err != nil { return err } - defer f.Close() + // since we are writing to this file, make sure we handle err on Close() + defer func() { + closeErr := f.Close() + if retErr == nil { + retErr = closeErr + } + }() encoder := toml.NewEncoder(f) return encoder.Encode(conf) diff --git a/sif/load.go b/sif/load.go index 70758ad4..43937b63 100644 --- a/sif/load.go +++ b/sif/load.go @@ -186,12 +186,18 @@ func convertSIFToElements(ctx context.Context, sifImage *sif.FileImage, tempDir // has an -o option that allows extracting a squashfs from the SIF file directly, // but that version is not currently available in RHEL 8. logrus.Debugf("Creating a temporary squashfs image %s ...", squashFSPath) - if err := func() error { // A scope for defer + if err := func() (retErr error) { // A scope for defer f, err := os.Create(squashFSPath) if err != nil { return err } - defer f.Close() + // since we are writing to this file, make sure we handle err on Close() + defer func() { + closeErr := f.Close() + if retErr == nil { + retErr = closeErr + } + }() // TODO: This can take quite some time, and should ideally be cancellable using ctx.Done(). if _, err := io.CopyN(f, rootFS.GetReader(), rootFS.Size()); err != nil { return err diff --git a/storage/storage_dest.go b/storage/storage_dest.go index 0af4523f..c750da93 100644 --- a/storage/storage_dest.go +++ b/storage/storage_dest.go @@ -272,43 +272,56 @@ func (s *storageImageDestination) putBlobToPendingFile(stream io.Reader, blobinf if err != nil { return private.UploadedBlob{}, fmt.Errorf("creating temporary file %q: %w", filename, err) } - defer file.Close() - counter := ioutils.NewWriteCounter(file) - stream = io.TeeReader(stream, counter) - digester, stream := putblobdigest.DigestIfUnknown(stream, blobinfo) - decompressed, err := archive.DecompressStream(stream) - if err != nil { - return private.UploadedBlob{}, fmt.Errorf("setting up to decompress blob: %w", err) - } + blobDigest, diffID, count, err := func() (_, _ digest.Digest, _ int64, retErr error) { // A scope for defer + // since we are writing to this file, make sure we handle err on Close() + defer func() { + closeErr := file.Close() + if retErr == nil { + retErr = closeErr + } + }() + counter := ioutils.NewWriteCounter(file) + stream = io.TeeReader(stream, counter) + digester, stream := putblobdigest.DigestIfUnknown(stream, blobinfo) + decompressed, err := archive.DecompressStream(stream) + if err != nil { + return "", "", 0, fmt.Errorf("setting up to decompress blob: %w", err) - diffID := digest.Canonical.Digester() - // Copy the data to the file. - // TODO: This can take quite some time, and should ideally be cancellable using context.Context. - _, err = io.Copy(diffID.Hash(), decompressed) - decompressed.Close() + } + defer decompressed.Close() + + diffID := digest.Canonical.Digester() + // Copy the data to the file. + // TODO: This can take quite some time, and should ideally be cancellable using context.Context. + _, err = io.Copy(diffID.Hash(), decompressed) + if err != nil { + return "", "", 0, fmt.Errorf("storing blob to file %q: %w", filename, err) + } + + return digester.Digest(), diffID.Digest(), counter.Count, nil + }() if err != nil { - return private.UploadedBlob{}, fmt.Errorf("storing blob to file %q: %w", filename, err) + return private.UploadedBlob{}, err } // Determine blob properties, and fail if information that we were given about the blob // is known to be incorrect. - blobDigest := digester.Digest() blobSize := blobinfo.Size if blobSize < 0 { - blobSize = counter.Count - } else if blobinfo.Size != counter.Count { + blobSize = count + } else if blobinfo.Size != count { return private.UploadedBlob{}, ErrBlobSizeMismatch } // Record information about the blob. s.lock.Lock() - s.lockProtected.blobDiffIDs[blobDigest] = diffID.Digest() - s.lockProtected.fileSizes[blobDigest] = counter.Count + s.lockProtected.blobDiffIDs[blobDigest] = diffID + s.lockProtected.fileSizes[blobDigest] = count s.lockProtected.filenames[blobDigest] = filename s.lock.Unlock() // This is safe because we have just computed diffID, and blobDigest was either computed // by us, or validated by the caller (usually copy.digestingReader). - options.Cache.RecordDigestUncompressedPair(blobDigest, diffID.Digest()) + options.Cache.RecordDigestUncompressedPair(blobDigest, diffID) return private.UploadedBlob{ Digest: blobDigest, Size: blobSize,