Skip to content

Commit 756540b

Browse files
committed
Validate OCI configs before unpacking.
Detect malformed image configs before calling umoci so recovered builds fail cleanly without relying on panic recovery for the captured diff_ids mismatch case. Made-with: Cursor
1 parent beacc2d commit 756540b

3 files changed

Lines changed: 33 additions & 24 deletions

File tree

lib/images/manager.go

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ import (
55
"fmt"
66
"os"
77
"path/filepath"
8-
"runtime/debug"
98
"sort"
109
"strings"
1110
"sync"
@@ -239,13 +238,6 @@ func (m *manager) buildImage(ctx context.Context, ref *ResolvedRef) {
239238
buildDir := m.paths.SystemBuild(ref.String())
240239
tempDir := filepath.Join(buildDir, "rootfs")
241240

242-
defer func() {
243-
if r := recover(); r != nil {
244-
m.updateStatusByDigest(ref, StatusFailed, fmt.Errorf("panic while building image %s: %v\n%s", ref.String(), r, debug.Stack()))
245-
m.recordBuildMetrics(ctx, buildStart, "failed")
246-
}
247-
}()
248-
249241
if err := os.MkdirAll(buildDir, 0755); err != nil {
250242
m.updateStatusByDigest(ref, StatusFailed, fmt.Errorf("create build dir: %w", err))
251243
m.recordBuildMetrics(ctx, buildStart, "failed")

lib/images/oci.go

Lines changed: 31 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ import (
55
"fmt"
66
"os"
77
"runtime"
8-
"runtime/debug"
98
"strings"
109

1110
"github.com/google/go-containerregistry/pkg/authn"
@@ -318,6 +317,14 @@ func (c *ociClient) unpackLayers(ctx context.Context, layoutTag, targetDir strin
318317
return fmt.Errorf("get manifest: %w", err)
319318
}
320319

320+
configFile, err := img.ConfigFile()
321+
if err != nil {
322+
return fmt.Errorf("get config file: %w", err)
323+
}
324+
if err := validateConfigFileForUnpack(layoutTag, gcrManifest, configFile); err != nil {
325+
return err
326+
}
327+
321328
// Convert go-containerregistry manifest to OCI v1.Manifest for umoci
322329
ociManifest := convertToOCIManifest(gcrManifest)
323330

@@ -352,26 +359,36 @@ func (c *ociClient) unpackLayers(ctx context.Context, layoutTag, targetDir strin
352359
},
353360
}
354361

355-
var panicErr error
356-
func() {
357-
defer func() {
358-
if r := recover(); r != nil {
359-
panicErr = fmt.Errorf("panic in unpack rootfs for %s: %v\n%s", layoutTag, r, debug.Stack())
360-
}
361-
}()
362-
363-
err = layer.UnpackRootfs(ctx, casEngine, targetDir, ociManifest, unpackOpts)
364-
}()
365-
if panicErr != nil {
366-
return panicErr
367-
}
362+
err = layer.UnpackRootfs(ctx, casEngine, targetDir, ociManifest, unpackOpts)
368363
if err != nil {
369364
return fmt.Errorf("unpack rootfs: %w", err)
370365
}
371366

372367
return nil
373368
}
374369

370+
func validateConfigFileForUnpack(layoutTag string, manifest *gcr.Manifest, configFile *gcr.ConfigFile) error {
371+
if convertToOCIMediaType(string(manifest.Config.MediaType)) != v1.MediaTypeImageConfig {
372+
return fmt.Errorf(
373+
"unpack rootfs: config blob is not correct mediatype %s: %s",
374+
v1.MediaTypeImageConfig,
375+
manifest.Config.MediaType,
376+
)
377+
}
378+
if configFile.RootFS.Type != "layers" {
379+
return fmt.Errorf("unpack rootfs: config: unsupported rootfs.type: %s", configFile.RootFS.Type)
380+
}
381+
if len(configFile.RootFS.DiffIDs) != len(manifest.Layers) {
382+
return fmt.Errorf(
383+
"unpack rootfs: config rootfs.diff_ids has %d entries but manifest has %d layers for %s",
384+
len(configFile.RootFS.DiffIDs),
385+
len(manifest.Layers),
386+
layoutTag,
387+
)
388+
}
389+
return nil
390+
}
391+
375392
// convertToOCIManifest converts a go-containerregistry manifest to OCI v1.Manifest
376393
// This allows us to use go-containerregistry (which handles both Docker v2 and OCI v1)
377394
// for manifest parsing, while still using umoci for layer unpacking.

lib/images/recovery_regression_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ func TestUnpackLayersCapturedFixtureReturnsErrorInsteadOfPanicking(t *testing.T)
3232
})
3333

3434
require.Error(t, unpackErr)
35-
assert.Contains(t, unpackErr.Error(), "panic in unpack rootfs")
35+
assert.Contains(t, unpackErr.Error(), "config rootfs.diff_ids has 0 entries but manifest has 1 layers")
3636
}
3737

3838
func TestRecoverInterruptedBuildsCapturedFixtureMarksBuildFailed(t *testing.T) {
@@ -64,7 +64,7 @@ func TestRecoverInterruptedBuildsCapturedFixtureMarksBuildFailed(t *testing.T) {
6464
require.NotNil(t, meta.Error)
6565
assert.Equal(t, recoveryFixtureDigest, meta.Digest)
6666
assert.Equal(t, StatusFailed, meta.Status)
67-
assert.Contains(t, *meta.Error, "panic in unpack rootfs")
67+
assert.Contains(t, *meta.Error, "config rootfs.diff_ids has 0 entries but manifest has 1 layers")
6868
}
6969

7070
func copyRecoveryFixture(t *testing.T) string {

0 commit comments

Comments
 (0)