image/docker/internal/tarfile: Fix size reporting for symlink layers#605
image/docker/internal/tarfile: Fix size reporting for symlink layers#6051seal wants to merge 1 commit intocontainers:mainfrom
Conversation
|
Packit jobs failed. @containers/packit-build please check. |
2 similar comments
|
Packit jobs failed. @containers/packit-build please check. |
|
Packit jobs failed. @containers/packit-build please check. |
mtrmac
left a comment
mtrmac
left a comment
There was a problem hiding this comment.
Thanks!
Note 785583c#sign-your-prs:
Use a real name (sorry, no anonymous contributions).
there’s nuance to that, please read on there.
image/docker/internal/tarfile/src.go
Outdated
| layerPath := path.Clean(h.Name) | ||
| // FIXME: Cache this data across images in Reader. | ||
| if li, ok := unknownLayerSizes[layerPath]; ok { | ||
| underlyingReader := io.Reader(t) |
There was a problem hiding this comment.
I think moving the contents of this if into a separate function would be easier.
Notably we can get rid of underlyingCloser and just use defer, much safer against the risk of forgetting to do that — and we would not have to stop using defer uncompressedStream.Close.
Even underlyingSize could be eliminated, in favor of h = resolvedHeader.
| // It is safe to call this method from multiple goroutines simultaneously. | ||
| // The caller should call .Close() on the returned stream. | ||
| func (r *Reader) openTarComponent(componentPath string) (io.ReadCloser, error) { | ||
| func (r *Reader) openTarComponentWithHeader(componentPath string) (io.ReadCloser, *tar.Header, error) { |
There was a problem hiding this comment.
openTarComponent has 2 callers; I think just having one function, adding a return value, and ignoring it in the 2 callers would be fine enough. It’s not worth having 2 version of openTarComponentWithHeader (and documenting the differences).
|
|
||
| var tarfileBuffer bytes.Buffer | ||
| tw := tar.NewWriter(&tarfileBuffer) | ||
| require.NoError(t, tw.WriteHeader(&tar.Header{ |
There was a problem hiding this comment.
Please keep the code that actually does something as separate statements from require, throughout.
| require.NoError(t, tw.WriteHeader(&tar.Header{ | ||
| Name: "manifest.json", | ||
| Mode: 0o644, | ||
| Size: int64(len(manifestBytes)), | ||
| })) | ||
| _, err = tw.Write(manifestBytes) | ||
| require.NoError(t, err) | ||
|
|
||
| require.NoError(t, tw.WriteHeader(&tar.Header{ | ||
| Name: "config.json", | ||
| Mode: 0o644, | ||
| Size: int64(len(configBytes)), | ||
| })) | ||
| _, err = tw.Write(configBytes) | ||
| require.NoError(t, err) | ||
|
|
||
| require.NoError(t, tw.WriteHeader(&tar.Header{ | ||
| Name: "layer.tar", | ||
| Mode: 0o644, | ||
| Size: int64(len(layerBytes)), | ||
| })) | ||
| _, err = tw.Write(layerBytes) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
These can be a loop over an array instead of repetitive code.
| reader, err := NewReaderFromStream(nil, &tarfileBuffer) | ||
| require.NoError(t, err) | ||
| src := NewSource(reader, true, "transport name", nil, -1) | ||
| t.Cleanup(func() { _ = src.Close() }) |
When a layer path in a docker-archive points at a symlink header, prepareLayerData recorded the symlink header size (often 0) while GetBlob followed the symlink and returned the target file bytes. Resolve a single symlink when determining the layer size so the reported size matches the bytes returned by GetBlob, and add a regression test. Signed-off-by: Oleh Konko <security@1seal.org>
1seal
force-pushed
the
fix-tarfile-symlink-layer-size
branch
from
785583c to
048739e
Compare
|
thanks for the detailed review
tests: go test ./... ; go vet ./... |
3 participants
This fixes a mismatch between the size returned by docker/internal/tarfile.Source.GetBlob and the bytes returned when reading docker-archive layers that are stored as symlink entries
(e.g. produced by
docker savefor duplicate layers).Previously, prepareLayerData recorded the symlink header size (often 0), while GetBlob followed the symlink and returned the target file bytes.
Changes:
Tests: