Whenever copying OCI Platform data, include OSVersion and OSFeatures

Trivially created by looking for every reference to .Variant and adding OSVersion and OSFeatures, except the ones related to the string representation of a Platform instance. I then went through and ensured every assignment of OSFeatures that might leak out, i.e., not local-only or for marhsalling purposes, uses the append-to-nil idiom to avoid sharing the slice storage and allowing accidental mutation after-the-fact. Signed-off-by: Paul "TBBle" Hampson <[email protected]>
moby · Nov 3, 2023 · 98e0d8d · 98e0d8d
1 parent 36c5550
commit 98e0d8d
Show file tree

Hide file tree

Showing 14 changed files with 98 additions and 39 deletions.
diff --git a/client/llb/marshal.go b/client/llb/marshal.go
@@ -95,14 +95,18 @@ func MarshalConstraints(base, override *Constraints) (*pb.Op, *pb.OpMetadata) {
 		c.Platform = &defaultPlatform
 	}
 
+	opPlatform := pb.Platform{
+		OS:           c.Platform.OS,
+		Architecture: c.Platform.Architecture,
+		Variant:      c.Platform.Variant,
+		OSVersion:    c.Platform.OSVersion,
+	}
+	if c.Platform.OSFeatures != nil {
+		opPlatform.OSFeatures = append([]string{}, c.Platform.OSFeatures...)
+	}
+
 	return &pb.Op{
-		Platform: &pb.Platform{
-			OS:           c.Platform.OS,
-			Architecture: c.Platform.Architecture,
-			Variant:      c.Platform.Variant,
-			OSVersion:    c.Platform.OSVersion,
-			OSFeatures:   c.Platform.OSFeatures,
-		},
+		Platform: &opPlatform,
 		Constraints: &pb.WorkerConstraints{
 			Filter: c.WorkerConstraints,
 		},

diff --git a/client/llb/state.go b/client/llb/state.go
@@ -258,11 +258,16 @@ func (s State) WithImageConfig(c []byte) (State, error) {
 	}
 	s = s.Dir(img.Config.WorkingDir)
 	if img.Architecture != "" && img.OS != "" {
-		s = s.Platform(ocispecs.Platform{
+		plat := ocispecs.Platform{
 			OS:           img.OS,
 			Architecture: img.Architecture,
 			Variant:      img.Variant,
-		})
+			OSVersion:    img.OSVersion,
+		}
+		if img.OSFeatures != nil {
+			plat.OSFeatures = append([]string{}, img.OSFeatures...)
+		}
+		s = s.Platform(plat)
 	}
 	return s, nil
 }

diff --git a/exporter/containerimage/writer.go b/exporter/containerimage/writer.go
@@ -598,6 +598,8 @@ func defaultImageConfig() ([]byte, error) {
 	img := ocispecs.Image{}
 	img.Architecture = pl.Architecture
 	img.OS = pl.OS
+	img.OSVersion = pl.OSVersion
+	img.OSFeatures = pl.OSFeatures
 	img.Variant = pl.Variant
 	img.RootFS.Type = "layers"
 	img.Config.WorkingDir = "/"

diff --git a/frontend/dockerfile/dockerfile2llb/convert.go b/frontend/dockerfile/dockerfile2llb/convert.go
@@ -274,6 +274,10 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS
 							OS:           img.OS,
 							Architecture: img.Architecture,
 							Variant:      img.Variant,
+							OSVersion:    img.OSVersion,
+						}
+						if img.OSFeatures != nil {
+							ds.platform.OSFeatures = append([]string{}, img.OSFeatures...)
 						}
 					}
 				}
@@ -595,6 +599,10 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS
 		target.image.OS = platformOpt.targetPlatform.OS
 		target.image.Architecture = platformOpt.targetPlatform.Architecture
 		target.image.Variant = platformOpt.targetPlatform.Variant
+		target.image.OSVersion = platformOpt.targetPlatform.OSVersion
+		if platformOpt.targetPlatform.OSFeatures != nil {
+			target.image.OSFeatures = append([]string{}, platformOpt.targetPlatform.OSFeatures...)
+		}
 	}
 
 	return target, nil

diff --git a/frontend/dockerfile/dockerfile2llb/image.go b/frontend/dockerfile/dockerfile2llb/image.go
@@ -19,6 +19,10 @@ func emptyImage(platform ocispecs.Platform) image.Image {
 	img := image.Image{}
 	img.Architecture = platform.Architecture
 	img.OS = platform.OS
+	img.OSVersion = platform.OSVersion
+	if platform.OSFeatures != nil {
+		img.OSFeatures = append([]string{}, platform.OSFeatures...)
+	}
 	img.Variant = platform.Variant
 	img.RootFS.Type = "layers"
 	img.Config.WorkingDir = "/"

diff --git a/frontend/dockerui/build.go b/frontend/dockerui/build.go
@@ -57,7 +57,7 @@ func (bc *Client) Build(ctx context.Context, fn BuildFunc) (*ResultBuilder, erro
 					p.OSVersion = img.OSVersion
 				}
 				if p.OSFeatures == nil && len(img.OSFeatures) > 0 {
-					p.OSFeatures = img.OSFeatures
+					p.OSFeatures = append([]string{}, img.OSFeatures...)
 				}
 			}
 

diff --git a/solver/llbsolver/ops/exec.go b/solver/llbsolver/ops/exec.go
@@ -113,6 +113,8 @@ func (e *ExecOp) CacheMap(ctx context.Context, g session.Group, index int) (*sol
 			OS:           e.platform.OS,
 			Architecture: e.platform.Architecture,
 			Variant:      e.platform.Variant,
+			OSVersion:    e.platform.OSVersion,
+			OSFeatures:   e.platform.OSFeatures,
 		}
 	}
 
@@ -133,17 +135,21 @@ func (e *ExecOp) CacheMap(ctx context.Context, g session.Group, index int) (*sol
 	}
 
 	dt, err := json.Marshal(struct {
-		Type    string
-		Exec    *pb.ExecOp
-		OS      string
-		Arch    string
-		Variant string `json:",omitempty"`
+		Type       string
+		Exec       *pb.ExecOp
+		OS         string
+		Arch       string
+		Variant    string   `json:",omitempty"`
+		OSVersion  string   `json:",omitempty"`
+		OSFeatures []string `json:",omitempty"`
 	}{
-		Type:    execCacheType,
-		Exec:    &op,
-		OS:      p.OS,
-		Arch:    p.Architecture,
-		Variant: p.Variant,
+		Type:       execCacheType,
+		Exec:       &op,
+		OS:         p.OS,
+		Arch:       p.Architecture,
+		Variant:    p.Variant,
+		OSVersion:  p.OSVersion,
+		OSFeatures: p.OSFeatures,
 	})
 	if err != nil {
 		return nil, false, err

diff --git a/solver/llbsolver/ops/exec_binfmt.go b/solver/llbsolver/ops/exec_binfmt.go
@@ -90,6 +90,8 @@ func getEmulator(ctx context.Context, p *pb.Platform, idmap *idtools.IdentityMap
 	pp := platforms.Normalize(ocispecs.Platform{
 		Architecture: p.Architecture,
 		OS:           p.OS,
+		OSVersion:    p.OSVersion,
+		OSFeatures:   p.OSFeatures,
 		Variant:      p.Variant,
 	})
 

diff --git a/solver/llbsolver/provenance/capture.go b/solver/llbsolver/provenance/capture.go
@@ -152,7 +152,9 @@ func (c *Capture) AddImage(i ImageSource) {
 				return
 			}
 			if v.Platform != nil && i.Platform != nil {
-				if v.Platform.Architecture == i.Platform.Architecture && v.Platform.OS == i.Platform.OS && v.Platform.Variant == i.Platform.Variant {
+				// NOTE: Deliberately excluding OSFeatures, as there's no extant (or rational) case where a source image is an index and contains images distinguished only by OSFeature
+				// See https://github.com/moby/buildkit/pull/4387#discussion_r1376234241 and https://github.com/opencontainers/image-spec/issues/1147
+				if v.Platform.Architecture == i.Platform.Architecture && v.Platform.OS == i.Platform.OS && v.Platform.OSVersion == i.Platform.OSVersion && v.Platform.Variant == i.Platform.Variant {
 					return
 				}
 			}

diff --git a/solver/llbsolver/vertex.go b/solver/llbsolver/vertex.go
@@ -79,17 +79,29 @@ func NormalizeRuntimePlatforms() LoadOpt {
 					OS:           p.OS,
 					Architecture: p.Architecture,
 					Variant:      p.Variant,
+					OSVersion:    p.OSVersion,
+					OSFeatures:   p.OSFeatures,
 				}
 			}
 			op.Platform = defaultPlatform
 		}
-		platform := ocispecs.Platform{OS: op.Platform.OS, Architecture: op.Platform.Architecture, Variant: op.Platform.Variant}
+		platform := ocispecs.Platform{
+			OS:           op.Platform.OS,
+			Architecture: op.Platform.Architecture,
+			Variant:      op.Platform.Variant,
+			OSVersion:    op.Platform.OSVersion,
+			OSFeatures:   op.Platform.OSFeatures,
+		}
 		normalizedPlatform := platforms.Normalize(platform)
 
 		op.Platform = &pb.Platform{
 			OS:           normalizedPlatform.OS,
 			Architecture: normalizedPlatform.Architecture,
 			Variant:      normalizedPlatform.Variant,
+			OSVersion:    normalizedPlatform.OSVersion,
+		}
+		if normalizedPlatform.OSFeatures != nil {
+			op.Platform.OSFeatures = append([]string{}, normalizedPlatform.OSFeatures...)
 		}
 
 		return nil

diff --git a/solver/pb/ops.proto b/solver/pb/ops.proto
@@ -29,7 +29,7 @@ message Platform {
 	string Architecture = 1;
 	string OS = 2;
 	string Variant = 3;
-	string OSVersion = 4; // unused
+	string OSVersion = 4;
 	repeated string OSFeatures = 5; // unused
 }
 

diff --git a/solver/pb/platform.go b/solver/pb/platform.go
@@ -5,23 +5,29 @@ import (
 )
 
 func (p *Platform) Spec() ocispecs.Platform {
-	return ocispecs.Platform{
+	result := ocispecs.Platform{
 		OS:           p.OS,
 		Architecture: p.Architecture,
 		Variant:      p.Variant,
 		OSVersion:    p.OSVersion,
-		OSFeatures:   p.OSFeatures,
 	}
+	if p.OSFeatures != nil {
+		result.OSFeatures = append([]string{}, p.OSFeatures...)
+	}
+	return result
 }
 
 func PlatformFromSpec(p ocispecs.Platform) Platform {
-	return Platform{
+	result := Platform{
 		OS:           p.OS,
 		Architecture: p.Architecture,
 		Variant:      p.Variant,
 		OSVersion:    p.OSVersion,
-		OSFeatures:   p.OSFeatures,
 	}
+	if p.OSFeatures != nil {
+		result.OSFeatures = append([]string{}, p.OSFeatures...)
+	}
+	return result
 }
 
 func ToSpecPlatforms(p []Platform) []ocispecs.Platform {

diff --git a/source/containerimage/pull.go b/source/containerimage/pull.go
@@ -60,17 +60,21 @@ type puller struct {
 
 func mainManifestKey(ctx context.Context, desc ocispecs.Descriptor, platform ocispecs.Platform, layerLimit *int) (digest.Digest, error) {
 	dt, err := json.Marshal(struct {
-		Digest  digest.Digest
-		OS      string
-		Arch    string
-		Variant string `json:",omitempty"`
-		Limit   *int   `json:",omitempty"`
+		Digest     digest.Digest
+		OS         string
+		Arch       string
+		Variant    string   `json:",omitempty"`
+		OSVersion  string   `json:",omitempty"`
+		OSFeatures []string `json:",omitempty"`
+		Limit      *int     `json:",omitempty"`
 	}{
-		Digest:  desc.Digest,
-		OS:      platform.OS,
-		Arch:    platform.Architecture,
-		Variant: platform.Variant,
-		Limit:   layerLimit,
+		Digest:     desc.Digest,
+		OS:         platform.OS,
+		Arch:       platform.Architecture,
+		Variant:    platform.Variant,
+		OSVersion:  platform.OSVersion,
+		OSFeatures: platform.OSFeatures,
+		Limit:      layerLimit,
 	})
 	if err != nil {
 		return "", err

diff --git a/source/containerimage/source.go b/source/containerimage/source.go
@@ -202,7 +202,9 @@ func (is *Source) registryIdentifier(ref string, attrs map[string]string, platfo
 			Architecture: platform.Architecture,
 			Variant:      platform.Variant,
 			OSVersion:    platform.OSVersion,
-			OSFeatures:   platform.OSFeatures,
+		}
+		if platform.OSFeatures != nil {
+			id.Platform.OSFeatures = append([]string{}, platform.OSFeatures...)
 		}
 	}
 
@@ -247,7 +249,9 @@ func (is *Source) ociIdentifier(ref string, attrs map[string]string, platform *p
 			Architecture: platform.Architecture,
 			Variant:      platform.Variant,
 			OSVersion:    platform.OSVersion,
-			OSFeatures:   platform.OSFeatures,
+		}
+		if platform.OSFeatures != nil {
+			id.Platform.OSFeatures = append([]string{}, platform.OSFeatures...)
 		}
 	}