From bcbb44651e6a095e38d7ee0055ea4ad87dfae575 Mon Sep 17 00:00:00 2001 From: Adam Eijdenberg Date: Sat, 22 Feb 2025 05:04:47 +0000 Subject: [PATCH] fix: handle error on Close() of writable objects For objects (particularly files) that we write to, we need to check the return value of f.Close() and pass error back to the caller if this fails. Signed-off-by: Adam Eijdenberg --- oci/archive/oci_dest.go | 11 +++++-- oci/layout/oci_delete.go | 10 ++++-- oci/layout/oci_dest.go | 19 ++++++++--- ostree/ostree_dest.go | 28 ++++++++++++---- pkg/blobinfocache/sqlite/sqlite.go | 33 ++++++++++++------- pkg/sysregistriesv2/shortnames.go | 10 ++++-- sif/load.go | 10 ++++-- storage/storage_dest.go | 53 +++++++++++++++++++----------- 8 files changed, 123 insertions(+), 51 deletions(-) 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,