Repository navigation
performance regression: GC() (or more generally, Walk()) now reads every blob off of disk and hashes it (twice) #373
Description
As of 4faf753 we now read every byte of every blob that's opened. Since the Walk() API uses FromDescriptor() to open a blob, every blob is opened during a GC, including data blobs. With the above patch, we end up hashing every byte in the image.
Further, FromDescriptor() uses GetVerifiedBlob(), which uses GetBlob(), both of which use a VerifiedReadCloser, so we actually end up checking each blob's hash twice during this verification.
This is a fairly severe performance regression from before, where we only verified blobs that were actually read in their entirety, IIUC.
Perhaps we can implement a .FSCK() which verifies blobs, and leave GC() to just do GC? Not clear how to do this given the current API that opens everything.
Stack trace for posterity:
goroutine 1 [runnable]:
github.com/pkg/errors.Cause(0x0, 0x0, 0x2000, 0x2000)
/home/tycho/packages/go/pkg/mod/github.com/pkg/errors@v0.9.1/errors.go:275 +0x9e
github.com/opencontainers/umoci/pkg/hardening.(*VerifiedReadCloser).Read(0xc0002eab40, 0xc000308000, 0x2000, 0x2000, 0x2000, 0x0, 0x0)
/home/tycho/packages/go/pkg/mod/github.com/opencontainers/umoci@v0.4.7/pkg/hardening/verified_reader.go:157 +0x185
github.com/opencontainers/umoci/pkg/hardening.(*VerifiedReadCloser).Read(0xc0002eab80, 0xc000308000, 0x2000, 0x2000, 0x2000, 0x0, 0x0)
/home/tycho/packages/go/pkg/mod/github.com/opencontainers/umoci@v0.4.7/pkg/hardening/verified_reader.go:127 +0xaa
io.discard.ReadFrom(0x1bd2200, 0xc0002eab80, 0x1130e40, 0xc000450401, 0x7fbf800209e0)
/usr/local/go/src/io/io.go:598 +0x95
io.copyBuffer(0x1bd4b40, 0x22abb00, 0x1bd2200, 0xc0002eab80, 0x0, 0x0, 0x0, 0xc0002eab80, 0x16, 0xc00099bfe8)
/usr/local/go/src/io/io.go:409 +0x357
io.Copy(...)
/usr/local/go/src/io/io.go:382
github.com/opencontainers/umoci/pkg/hardening.(*VerifiedReadCloser).Close(0xc0002eab80, 0x1144c00, 0xc0002eab80)
/home/tycho/packages/go/pkg/mod/github.com/opencontainers/umoci@v0.4.7/pkg/hardening/verified_reader.go:185 +0x75
github.com/opencontainers/umoci/oci/casext.(*Blob).Close(...)
/home/tycho/packages/go/pkg/mod/github.com/opencontainers/umoci@v0.4.7/oci/casext/blob.go:56
github.com/opencontainers/umoci/oci/casext.(*walkState).recurse.func1(0xc00059f1a0, 0xc000450480, 0x2c, 0xc0007b6820, 0x47, 0x22a2000, 0x0, 0x0, 0x0, 0x0, ...)
/home/tycho/packages/go/pkg/mod/github.com/opencontainers/umoci@v0.4.7/oci/casext/walk.go:133 +0x1d0
github.com/opencontainers/umoci/oci/casext.(*walkState).recurse(0xc00099c728, 0x1be6f58, 0xc0001bc798, 0xc000481f40, 0x2, 0x2, 0x0, 0x0)
/home/tycho/packages/go/pkg/mod/github.com/opencontainers/umoci@v0.4.7/oci/casext/walk.go:150 +0x9fd
github.com/opencontainers/umoci/oci/casext.(*walkState).recurse(0xc00099c728, 0x1be6f58, 0xc0001bc798, 0xc000616eb0, 0x1, 0x1, 0x0, 0x0)
/home/tycho/packages/go/pkg/mod/github.com/opencontainers/umoci@v0.4.7/oci/casext/walk.go:143 +0x96f
github.com/opencontainers/umoci/oci/casext.Engine.Walk(...)
/home/tycho/packages/go/pkg/mod/github.com/opencontainers/umoci@v0.4.7/oci/casext/walk.go:162
github.com/opencontainers/umoci/oci/casext.Engine.reachable(0x1befa68, 0xc0006c1c50, 0x1be6f58, 0xc0001bc798, 0xc000482570, 0x2a, 0xc0001c4fa0, 0x47, 0xd9c, 0x0, ...)
/home/tycho/packages/go/pkg/mod/github.com/opencontainers/umoci@v0.4.7/oci/casext/walk.go:177 +0x15b
github.com/opencontainers/umoci/oci/casext.Engine.GC(0x1befa68, 0xc0006c1c50, 0x1be6f58, 0xc0001bc798, 0x0, 0x0, 0x0, 0x2a, 0xc000758820)
/home/tycho/packages/go/pkg/mod/github.com/opencontainers/umoci@v0.4.7/oci/casext/gc.go:69 +0x518
Activity
deps: revert umoci to pre-v0.4.7
deps: revert umoci to pre-v0.4.7
deps: revert umoci to pre-v0.4.7
cyphar commentedon Aug 3, 2021
Yeah I noticed this when I first wrote it but wasn't quite sure how to fix it at the time (and since it was a security hardening I felt it more important to have the fix than figure out a workaround) -- I think that we need to have some kind of stat API in the CAS, and then we only read the blob at all during walking if the blob is a media type we can descend into. I was a bit worried this'd make the walk code less generic (meaning that library users who register custom blob types would have issues) but I think we can handle that.
VerifiedReadCloser should handle nesting without an issue -- it detects if the thing it's wrapping is also a VerifiedReadCloser and if it has the same verify attributes. If it does, it's a no-op and doesn't hash anything.
cyphar commentedon Aug 4, 2021
go.mod: update umoci dep
go.mod: update umoci dep
Updated code coverage (#7)