Skip to content

Fix issues from container build digest code review #55322

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
lbussell wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Fix issues from container build digest code review #55322

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3971e36
Dispose cached blob stream after digest validation
lbussell Jul 16, 2026
072b05a
Name cached digest log properties
lbussell Jul 16, 2026
2aa83ec
Remove unused digest validation helper
lbussell Jul 16, 2026
792facf
Test registry blob digest validation
lbussell Jul 16, 2026
4c408ca
Clean up failed container temporary files
lbussell Jul 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 0 additions & 9 deletions src/Containers/Microsoft.NET.Build.Containers/DigestUtils.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,15 +113,6 @@ internal static string ComputeSha256(string content)
return Convert.ToHexStringLower(hash);
}

/// <summary>
/// Validates hash value against the expected hash, failing with a
/// consistent error message if they don't match.
/// </summary>
internal static void ValidateHashValueAsync(ReadOnlySpan<byte> actualHash, ReadOnlySpan<byte> expectedHash)
{
InvalidDigestException.ThrowIfMismatched(expectedHash, actualHash);
}

/// <summary>
/// Validates a digest string against the OCI grammar and registered
/// algorithms, returning the parsed algorithm and encoded portions. Throws
Expand Down
225 changes: 116 additions & 109 deletions src/Containers/Microsoft.NET.Build.Containers/Layer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,139 +89,146 @@ public static Layer FromDirectory(string directory, string containerPath, bool i
}

string tempTarballPath = ContentStore.GetTempFile();
using (FileStream fs = File.Create(tempTarballPath))
try
{
using (HashDigestGZipStream gz = new(fs, leaveOpen: true))
using (FileStream fs = File.Create(tempTarballPath))
{
using (TarWriter writer = new(gz, TarEntryFormat.Pax, leaveOpen: true))
using (HashDigestGZipStream gz = new(fs, leaveOpen: true))
{
// Windows layers need a Files folder
if (isWindowsLayer)
using (TarWriter writer = new(gz, TarEntryFormat.Pax, leaveOpen: true))
{
var entry = new PaxTarEntry(TarEntryType.Directory, "Files", entryAttributes);
writer.WriteEntry(entry);
}

// Write an entry for the application directory.
WriteTarEntryForFile(writer, new DirectoryInfo(directory), containerPath, entryAttributes, isWindowsLayer ? null : userId);

// Write entries for the application directory contents.
var fileList = new FileSystemEnumerable<(FileSystemInfo file, string containerPath)>(
directory: directory,
transform: (ref FileSystemEntry entry) =>
{
FileSystemInfo fsi = entry.ToFileSystemInfo();
string relativePath = Path.GetRelativePath(directory, fsi.FullName);
if (OperatingSystem.IsWindows())
// Windows layers need a Files folder
if (isWindowsLayer)
{
var entry = new PaxTarEntry(TarEntryType.Directory, "Files", entryAttributes);
writer.WriteEntry(entry);
}

// Write an entry for the application directory.
WriteTarEntryForFile(writer, new DirectoryInfo(directory), containerPath, entryAttributes, isWindowsLayer ? null : userId);

// Write entries for the application directory contents.
var fileList = new FileSystemEnumerable<(FileSystemInfo file, string containerPath)>(
directory: directory,
transform: (ref FileSystemEntry entry) =>
{
// Use only '/' directory separators.
relativePath = relativePath.Replace('\\', '/');
}
return (fsi, $"{containerPath}/{relativePath}");
},
options: new EnumerationOptions()
{
AttributesToSkip = FileAttributes.System, // Include hidden files
RecurseSubdirectories = true
});
foreach (var item in fileList)
{
WriteTarEntryForFile(writer, item.file, item.containerPath, entryAttributes, isWindowsLayer ? null : userId);
}

// Windows layers need a Hives folder, we do not need to create any Registry Hive deltas inside
if (isWindowsLayer)
{
var entry = new PaxTarEntry(TarEntryType.Directory, "Hives", entryAttributes);
writer.WriteEntry(entry);
}

} // Dispose of the TarWriter before getting the hash so the final data get written to the tar stream

int bytesWritten = gz.GetCurrentUncompressedHash(uncompressedHash);
Debug.Assert(bytesWritten == uncompressedHash.Length);
}
FileSystemInfo fsi = entry.ToFileSystemInfo();
string relativePath = Path.GetRelativePath(directory, fsi.FullName);
if (OperatingSystem.IsWindows())
{
// Use only '/' directory separators.
relativePath = relativePath.Replace('\\', '/');
}
return (fsi, $"{containerPath}/{relativePath}");
},
options: new EnumerationOptions()
{
AttributesToSkip = FileAttributes.System, // Include hidden files
RecurseSubdirectories = true
});
foreach (var item in fileList)
{
WriteTarEntryForFile(writer, item.file, item.containerPath, entryAttributes, isWindowsLayer ? null : userId);
}

// Windows layers need a Hives folder, we do not need to create any Registry Hive deltas inside
if (isWindowsLayer)
{
var entry = new PaxTarEntry(TarEntryType.Directory, "Hives", entryAttributes);
writer.WriteEntry(entry);
}

} // Dispose of the TarWriter before getting the hash so the final data get written to the tar stream

int bytesWritten = gz.GetCurrentUncompressedHash(uncompressedHash);
Debug.Assert(bytesWritten == uncompressedHash.Length);
}

fileSize = fs.Length;
fileSize = fs.Length;

fs.Position = 0;
fs.Position = 0;

int bW = SHA256.HashData(fs, hash);
Debug.Assert(bW == hash.Length);
int bW = SHA256.HashData(fs, hash);
Debug.Assert(bW == hash.Length);

// Writes a tar entry corresponding to the file system item.
static void WriteTarEntryForFile(TarWriter writer, FileSystemInfo file, string containerPath, IEnumerable<KeyValuePair<string, string>> entryAttributes, int? userId)
{
UnixFileMode mode = DetermineFileMode(file);
PaxTarEntry entry;

if (file is FileInfo)
// Writes a tar entry corresponding to the file system item.
static void WriteTarEntryForFile(TarWriter writer, FileSystemInfo file, string containerPath, IEnumerable<KeyValuePair<string, string>> entryAttributes, int? userId)
{
var fileStream = File.OpenRead(file.FullName);
entry = new(TarEntryType.RegularFile, containerPath, entryAttributes)
UnixFileMode mode = DetermineFileMode(file);
PaxTarEntry entry;

if (file is FileInfo)
{
DataStream = fileStream,
};
}
else
{
entry = new(TarEntryType.Directory, containerPath, entryAttributes);
}
var fileStream = File.OpenRead(file.FullName);
entry = new(TarEntryType.RegularFile, containerPath, entryAttributes)
{
DataStream = fileStream,
};
}
else
{
entry = new(TarEntryType.Directory, containerPath, entryAttributes);
}

entry.Mode = mode;
if (userId is int uid)
{
entry.Uid = uid;
}
entry.Mode = mode;
if (userId is int uid)
{
entry.Uid = uid;
}

writer.WriteEntry(entry);
writer.WriteEntry(entry);

if (entry.DataStream is not null)
{
// no longer relying on the `using` of the FileStream, so need to do it manually
entry.DataStream.Dispose();
}
if (entry.DataStream is not null)
{
// no longer relying on the `using` of the FileStream, so need to do it manually
Comment on lines +179 to +183
entry.DataStream.Dispose();
}

static UnixFileMode DetermineFileMode(FileSystemInfo file)
{
const UnixFileMode nonExecuteMode = UnixFileMode.UserRead | UnixFileMode.UserWrite |
UnixFileMode.GroupRead |
UnixFileMode.OtherRead;
const UnixFileMode executeMode = nonExecuteMode | UnixFileMode.UserExecute | UnixFileMode.GroupExecute | UnixFileMode.OtherExecute;

// On Unix, we can determine the x-bit based on the filesystem permission.
// On Windows, we use executable permissions for all entries.
return (OperatingSystem.IsWindows() || ((file.UnixFileMode | UnixFileMode.UserExecute) != 0)) ? executeMode : nonExecuteMode;
static UnixFileMode DetermineFileMode(FileSystemInfo file)
{
const UnixFileMode nonExecuteMode = UnixFileMode.UserRead | UnixFileMode.UserWrite |
UnixFileMode.GroupRead |
UnixFileMode.OtherRead;
const UnixFileMode executeMode = nonExecuteMode | UnixFileMode.UserExecute | UnixFileMode.GroupExecute | UnixFileMode.OtherExecute;

// On Unix, we can determine the x-bit based on the filesystem permission.
// On Windows, we use executable permissions for all entries.
return (OperatingSystem.IsWindows() || ((file.UnixFileMode | UnixFileMode.UserExecute) != 0)) ? executeMode : nonExecuteMode;
}
}
}
}

string contentHash = Convert.ToHexStringLower(hash);
string uncompressedContentHash = Convert.ToHexStringLower(uncompressedHash);
string contentHash = Convert.ToHexStringLower(hash);
string uncompressedContentHash = Convert.ToHexStringLower(uncompressedHash);

string layerMediaType = manifestMediaType switch
{
// TODO: configurable? gzip always?
SchemaTypes.DockerManifestV2 => SchemaTypes.DockerLayerGzip,
SchemaTypes.OciManifestV1 => SchemaTypes.OciLayerGzipV1,
_ => throw new ArgumentException(Resource.FormatString(nameof(Strings.UnrecognizedMediaType), manifestMediaType))
};
string layerMediaType = manifestMediaType switch
{
// TODO: configurable? gzip always?
SchemaTypes.DockerManifestV2 => SchemaTypes.DockerLayerGzip,
SchemaTypes.OciManifestV1 => SchemaTypes.OciLayerGzipV1,
_ => throw new ArgumentException(Resource.FormatString(nameof(Strings.UnrecognizedMediaType), manifestMediaType))
};

Descriptor descriptor = new()
{
MediaType = layerMediaType,
Size = fileSize,
Digest = $"sha256:{contentHash}",
UncompressedDigest = $"sha256:{uncompressedContentHash}",
};
Descriptor descriptor = new()
{
MediaType = layerMediaType,
Size = fileSize,
Digest = $"sha256:{contentHash}",
UncompressedDigest = $"sha256:{uncompressedContentHash}",
};

string storedContent = ContentStore.PathForDescriptor(descriptor);
string storedContent = ContentStore.PathForDescriptor(descriptor);

Directory.CreateDirectory(ContentStore.ContentRoot);
Directory.CreateDirectory(ContentStore.ContentRoot);

File.Move(tempTarballPath, storedContent, overwrite: true);
File.Move(tempTarballPath, storedContent, overwrite: true);

return new(storedContent, descriptor);
return new(storedContent, descriptor);
}
finally
{
File.Delete(tempTarballPath);
}
Comment on lines +228 to +231

@baronfel baronfel Jul 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these try/catches for deletes are really bad whitespace-wise. maybe a structure that wraps a file-create in a Disposable so it can be deleted-on-leaving-scope in a clean way would be easier to read and use correctly without touching so much code?

}

internal virtual Stream OpenBackingFile() => File.OpenRead(BackingFile);
Expand Down
63 changes: 35 additions & 28 deletions src/Containers/Microsoft.NET.Build.Containers/Registry/Registry.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -414,7 +414,7 @@ public async Task<string> DownloadBlobAsync(string repository, Descriptor descri

try
{
var fileStream = File.OpenRead(localPath);
using var fileStream = File.OpenRead(localPath);

var actualHash = SHA256.HashData(fileStream);
var expectedHash = DigestUtils.GetEncodedValue(descriptor.Digest);
Expand All @@ -434,48 +434,55 @@ public async Task<string> DownloadBlobAsync(string repository, Descriptor descri
{
// Incorrect digest
_logger.LogTrace(
"Digest validation failed for cached blob {1} ({2}), redownloading from registry.",

@baronfel baronfel Jul 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this correct? I think this is using .NET's String.Format placeholders, which only accept numeric placeholders.

"Digest validation failed for cached blob {Path} ({Error}), redownloading from registry.",
localPath, exception.Message);
}

string tempTarballPath = ContentStore.GetTempFile();

int retryCount = 0;
while (retryCount < MaxDownloadRetries)
try
{
try
int retryCount = 0;
while (retryCount < MaxDownloadRetries)
{
// No local copy, so download one
using Stream responseStream = await _registryAPI.Blob.GetStreamAsync(repository, descriptor.Digest, cancellationToken).ConfigureAwait(false);

using (FileStream fs = File.Create(tempTarballPath))
try
{
await responseStream
.CopyToAndVerifyAsync(fs, descriptor.Digest, cancellationToken)
.ConfigureAwait(false);
// No local copy, so download one
using Stream responseStream = await _registryAPI.Blob.GetStreamAsync(repository, descriptor.Digest, cancellationToken).ConfigureAwait(false);

using (FileStream fs = File.Create(tempTarballPath))
{
await responseStream
.CopyToAndVerifyAsync(fs, descriptor.Digest, cancellationToken)
.ConfigureAwait(false);
}

// Break the loop if successful
break;
}

// Break the loop if successful
break;
}
catch (Exception ex)
{
retryCount++;
if (retryCount >= MaxDownloadRetries)
catch (Exception ex)

@mthalman mthalman Jul 17, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will catch cancellation exceptions. Those should bubble up as-is.

@baronfel baronfel Jul 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree

{
throw new UnableToDownloadFromRepositoryException(repository);
}
retryCount++;
if (retryCount >= MaxDownloadRetries)
{
throw new UnableToDownloadFromRepositoryException(repository);
}

_logger.LogTrace("Download attempt {0}/{1} for repository '{2}' failed. Error: {3}", retryCount, MaxDownloadRetries, repository, ex.ToString());
_logger.LogTrace("Download attempt {0}/{1} for repository '{2}' failed. Error: {3}", retryCount, MaxDownloadRetries, repository, ex.ToString());

// Wait before retrying
await Task.Delay(_retryDelayProvider(), cancellationToken).ConfigureAwait(false);
// Wait before retrying
await Task.Delay(_retryDelayProvider(), cancellationToken).ConfigureAwait(false);
}
}
}

File.Move(tempTarballPath, localPath, overwrite: true);
File.Move(tempTarballPath, localPath, overwrite: true);

return localPath;
return localPath;
}
finally
{
File.Delete(tempTarballPath);

@baronfel baronfel Jul 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similar comment here: this is high-touch and a Disposable file would solve it more cleanly for review purposes.

}
Comment on lines +482 to +485
}

internal async Task PushLayerAsync(Layer layer, string repository, CancellationToken cancellationToken)
Expand Down
33 changes: 33 additions & 0 deletions test/Microsoft.NET.Build.Containers.UnitTests/LayerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

namespace Microsoft.NET.Build.Containers.UnitTests;

[TestClass]
public class LayerTests
{
[TestMethod]
[DoNotParallelize]
public void FromDirectory_DeletesTempFileWhenCreationFails()
{
DirectoryInfo artifactRoot = Directory.CreateTempSubdirectory();
DirectoryInfo layerContent = Directory.CreateTempSubdirectory();
string priorArtifactRoot = ContentStore.ArtifactRoot;

try
{
ContentStore.ArtifactRoot = artifactRoot.FullName;

Assert.ThrowsExactly<ArgumentException>(() =>
Layer.FromDirectory(layerContent.FullName, "/app", false, "unsupported"));

Assert.IsEmpty(Directory.EnumerateFiles(ContentStore.TempPath));
}
finally
{
ContentStore.ArtifactRoot = priorArtifactRoot;
artifactRoot.Delete(recursive: true);
layerContent.Delete(recursive: true);
}
}
}
Loading
Loading