nik/navidrome
1
0
Fork 0
mirror of https://github.com/navidrome/navidrome.git synced 2026-08-01 07:21:17 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(artwork): bound decoded dimensions and stop the clamp at serve start

Browse Source 
Two hardening fixes to the inline updater. The tee's 20MB cap bounds compressed
input only, so a small file declaring a huge raster could allocate GBs on decode;
DecodeConfig now rejects anything over ~36M pixels from the header alone. And the
write-side version clamp no longer advances past the serve's start time: a version
change that lands mid-serve is not provably covered by the bytes being streamed,
so the clamp stops there, the DTO omits, and the next serve of the new bytes
heals — while structural read-side over-approximation (which always predates the
serve) still clamps fully.
This commit is contained in:
Deluan 2026-07-17 23:53:22 -04:00
parent cddc30b586
commit 116fc5b853
3 changed files with 73 additions and 18 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
core/artwork/artwork.go
View File

@ -93,8 +93,9 @@ func (a *artwork) Get(ctx context.Context, artID model.ArtworkID, size int, squa
// changes precisely when the served cover changes. Placeholder bytes (playlist fallback) clear.
// The tee wraps r directly, so Close reaches the underlying stream (no fd leak).
version := capAtNow(artReader.LastUpdated())
start := time.Now()
reader = newTeeReader(r, maxTeeBytes,
func(data []byte) { a.blurHashes.update(ctx, artID, data, version) })
func(data []byte) { a.blurHashes.update(ctx, artID, data, version, start) })
}
return reader, artReader.LastUpdated(), nil
}

23
core/artwork/blurhash_updater.go
View File

@ -46,9 +46,13 @@ func eligibleKind(artID model.ArtworkID) bool {
return false
}
// maxDecodePixels bounds the decoded raster: the tee's byte cap limits compressed size only, and a
// small file can declare huge dimensions that would allocate GBs on decode (decompression bomb).
const maxDecodePixels = 36_000_000 // ~6000x6000; decoded RGBA tops out around 144MB
// update hashes the exact bytes served for artID and persists the result. Placeholder bytes mean the
// entity has no artwork anymore, so they clear a stored hash instead.
func (u *blurHashUpdater) update(ctx context.Context, artID model.ArtworkID, data []byte, version time.Time) {
// entity has no artwork anymore, so they clear a stored hash instead. start is when the serve began.
func (u *blurHashUpdater) update(ctx context.Context, artID model.ArtworkID, data []byte, version, start time.Time) {
// Decoding arbitrary image bytes can panic; the serve already succeeded, so just log it.
defer func() {
if r := recover(); r != nil {
@ -66,9 +70,14 @@ func (u *blurHashUpdater) update(ctx context.Context, artID model.ArtworkID, dat
sum := checksum(data)
hash := u.cachedHash(artID, sum)
if hash == "" {
cfg, _, err := image.DecodeConfig(bytes.NewReader(data))
if err != nil || cfg.Width*cfg.Height > maxDecodePixels {
// Undecodable or oversized served bytes are not proof of change; keep the stored hash.
log.Trace(ctx, "BlurHash: skipping served bytes", "artID", artID, "width", cfg.Width, "height", cfg.Height, err)
return
}
img, _, err := image.Decode(bytes.NewReader(data))
if err != nil {
// Undecodable served bytes are not proof of change; leave the stored hash intact.
log.Trace(ctx, "BlurHash: served bytes not decodable, keeping stored hash", "artID", artID, err)
return
}
@ -82,12 +91,16 @@ func (u *blurHashUpdater) update(ctx context.Context, artID model.ArtworkID, dat
if err != nil {
return
}
// Clamp the persisted version up to the entity's, but never past the serve's start: a version that
// predates the serve is provably covered by the served bytes, one that landed mid-serve is not —
// there the clamp stops, the DTO omits, and the next serve of the new bytes heals.
if entityVersion.After(start) {
entityVersion = start
}
if stored == hash && storedAt != nil && !storedAt.Before(entityVersion) {
u.remember(artID, blurHashState{sum: sum, hash: hash})
return
}
// Clamp the persisted version up to the entity's: the hash is fresh for what is served right now,
// so the DTO accepts it after any serve, however much the read-side version over-approximates.
target := capAtNow(utils.TimeNewest(version, entityVersion))
if err := u.persist(ctx, artID, hash, target); err != nil {
log.Warn(ctx, "BlurHash: error persisting", "artID", artID, err)

65
core/artwork/blurhash_updater_internal_test.go
View File

@ -2,6 +2,8 @@ package artwork
import (
"bytes"
"encoding/binary"
"hash/crc32"
"image"
"image/color"
"image/png"
@ -13,6 +15,25 @@ import (
. "github.com/onsi/gomega"
)
// hugePNGHeader builds a valid PNG signature+IHDR declaring a 50000x50000 raster with no pixel data:
// enough for DecodeConfig to report the dimensions the decode gate must reject.
func hugePNGHeader() []byte {
var buf bytes.Buffer
buf.Write([]byte{0x89, 'P', 'N', 'G', 0x0d, 0x0a, 0x1a, 0x0a})
ihdr := make([]byte, 13)
binary.BigEndian.PutUint32(ihdr[0:], 50000)
binary.BigEndian.PutUint32(ihdr[4:], 50000)
ihdr[8] = 8 // bit depth
ihdr[9] = 6 // RGBA
var chunk bytes.Buffer
chunk.WriteString("IHDR")
chunk.Write(ihdr)
_ = binary.Write(&buf, binary.BigEndian, uint32(13))
buf.Write(chunk.Bytes())
_ = binary.Write(&buf, binary.BigEndian, crc32.ChecksumIEEE(chunk.Bytes()))
return buf.Bytes()
}
// pngImage builds a deterministic 2x2 PNG image for a label (color derived from label bytes).
func pngImage(label string) *image.RGBA {
img := image.NewRGBA(image.Rect(0, 0, 2, 2))
@ -61,7 +82,7 @@ var _ = Describe("blurHashUpdater", func() {
It("persists a hash computed from the served bytes", func() {
id := album(model.Album{ID: "al-1", UpdatedAt: version})
u.update(GinkgoT().Context(), id, realPNGBytes("x"), version)
u.update(GinkgoT().Context(), id, realPNGBytes("x"), version, time.Now())
al := stored("al-1")
Expect(al.BlurHash).ToNot(BeEmpty())
Expect(al.BlurHashUpdatedAt).To(HaveValue(Equal(version)))
@ -69,23 +90,23 @@ var _ = Describe("blurHashUpdater", func() {
It("clears the stored hash when the served bytes are a placeholder", func() {
id := album(model.Album{ID: "al-1", UpdatedAt: version, BlurHash: "OLD"})
u.update(GinkgoT().Context(), id, placeholderImages()[0], version)
u.update(GinkgoT().Context(), id, placeholderImages()[0], version, time.Now())
Expect(stored("al-1").BlurHash).To(BeEmpty())
})
It("leaves the hash untouched on undecodable bytes", func() {
id := album(model.Album{ID: "al-1", UpdatedAt: version, BlurHash: "KEEP"})
u.update(GinkgoT().Context(), id, []byte("not an image"), version)
u.update(GinkgoT().Context(), id, []byte("not an image"), version, time.Now())
Expect(stored("al-1").BlurHash).To(Equal("KEEP"))
})
It("does not rewrite when the stored hash is current for the entity version", func() {
id := album(model.Album{ID: "al-1", UpdatedAt: version})
data := realPNGBytes("dedup")
u.update(GinkgoT().Context(), id, data, version)
u.update(GinkgoT().Context(), id, data, version, time.Now())
first := stored("al-1")
// A later serve of the same bytes (newer tee version, unchanged entity) must not move the row.
u.update(GinkgoT().Context(), id, data, version.Add(time.Hour))
u.update(GinkgoT().Context(), id, data, version.Add(time.Hour), time.Now())
Expect(stored("al-1").BlurHashUpdatedAt).To(HaveValue(Equal(*first.BlurHashUpdatedAt)))
})
@ -94,12 +115,12 @@ var _ = Describe("blurHashUpdater", func() {
// by construction, so the write clamps up and the DTO accepts it — omission windows close.
id := album(model.Album{ID: "al-1", UpdatedAt: version})
data := realPNGBytes("clamp")
u.update(GinkgoT().Context(), id, data, version)
u.update(GinkgoT().Context(), id, data, version, time.Now())
first := stored("al-1")
newer := version.Add(time.Hour)
album(model.Album{ID: "al-1", UpdatedAt: newer, BlurHash: first.BlurHash, BlurHashUpdatedAt: first.BlurHashUpdatedAt})
u.update(GinkgoT().Context(), id, data, version) // same bytes, old tee version
u.update(GinkgoT().Context(), id, data, version, time.Now()) // same bytes, old tee version
second := stored("al-1")
Expect(second.BlurHash).To(Equal(first.BlurHash))
Expect(second.BlurHashUpdatedAt).To(HaveValue(Equal(newer)))
@ -108,16 +129,36 @@ var _ = Describe("blurHashUpdater", func() {
It("restores the stored hash when it drifts from the served bytes", func() {
id := album(model.Album{ID: "al-1", UpdatedAt: version})
data := realPNGBytes("truth")
u.update(GinkgoT().Context(), id, data, version)
u.update(GinkgoT().Context(), id, data, version, time.Now())
truth := stored("al-1").BlurHash
Expect(repo.UpdateBlurHash("al-1", "DRIFTED", version)).To(Succeed())
u.update(GinkgoT().Context(), id, data, version)
u.update(GinkgoT().Context(), id, data, version, time.Now())
Expect(stored("al-1").BlurHash).To(Equal(truth))
})
It("skips images whose declared dimensions exceed the decode bound", func() {
// The tee's byte cap limits compressed size only; a decompression bomb must be rejected from
// the header before the raster is allocated.
id := album(model.Album{ID: "al-1", UpdatedAt: version, BlurHash: "KEEP"})
u.update(GinkgoT().Context(), id, hugePNGHeader(), version, time.Now())
Expect(stored("al-1").BlurHash).To(Equal("KEEP"))
})
It("does not mark older served bytes as current when the version advances mid-serve", func() {
// The cover was replaced and scanned after this serve started: the clamp must stop at the
// serve's start, so the DTO keeps omitting until the new bytes are served.
changedAt := version.Add(time.Hour)
id := album(model.Album{ID: "al-1", UpdatedAt: changedAt})
u.update(GinkgoT().Context(), id, realPNGBytes("old-bytes"), version, version)
al := stored("al-1")
Expect(al.BlurHash).ToNot(BeEmpty())
Expect(al.BlurHashUpdatedAt).To(HaveValue(Equal(version)))
Expect(al.BlurHashUpdatedAt.Before(al.ArtworkUpdatedAt())).To(BeTrue(), "must read as stale")
})
It("does not write when a placeholder is served and nothing was ever stored", func() {
id := album(model.Album{ID: "al-1", UpdatedAt: version})
u.update(GinkgoT().Context(), id, placeholderImages()[0], version)
u.update(GinkgoT().Context(), id, placeholderImages()[0], version, time.Now())
Expect(stored("al-1").BlurHashUpdatedAt).To(BeNil())
})
@ -131,10 +172,10 @@ var _ = Describe("blurHashUpdater", func() {
It("keys the decode cache by identity, ignoring the artwork id's embedded timestamp", func() {
id := album(model.Album{ID: "al-1", UpdatedAt: version})
data := realPNGBytes("dedup")
u.update(GinkgoT().Context(), id, data, version)
u.update(GinkgoT().Context(), id, data, version, time.Now())
bumped := id
bumped.LastUpdate = version.Add(time.Hour)
u.update(GinkgoT().Context(), bumped, data, version)
u.update(GinkgoT().Context(), bumped, data, version, time.Now())
Expect(u.seen).To(HaveLen(1))
})
})