perf(kittygraphics): decode base64 once and avoid payload copies
DecodeBase64 picks padded or raw decoding from the presence of '=' and decodes straight into a right-sized buffer, with identical accept/reject set and error text (property and fuzz tested against the previous implementation). Add ParseAPCBorrowed and Scene.AddAssetOwned/ReplaceAssetOwned so the freshly decoded image bytes move into the scene without extra copies; public copying entry points are unchanged.
- commit
- 2234949c57608b9ae642a6d91fc4c718f206a2f5
- author
- brice · · 2026-10-02 09:33 UTC
- parent
- dca8245
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
5 files · +181 -19
M graphics/scene.go +29 -5
| @@ -101,13 +101,24 @@ func (s *Scene) Limits() Limits { | |||
| 101 | 101 | ||
| 102 | 102 | // AddAsset copies and registers an encoded asset, returning an opaque ID. | |
| 103 | 103 | func (s *Scene) AddAsset(blob AssetBlob) (AssetID, error) { | |
| 104 | + | return s.addAsset(blob, true) | |
| 105 | + | } | |
| 106 | + | ||
| 107 | + | // AddAssetOwned is AddAsset for callers that hand over blob.Encoded: the scene | |
| 108 | + | // keeps the slice without copying, so the caller must not read or modify it | |
| 109 | + | // afterwards. Snapshots and AssetView still return copies. | |
| 110 | + | func (s *Scene) AddAssetOwned(blob AssetBlob) (AssetID, error) { | |
| 111 | + | return s.addAsset(blob, false) | |
| 112 | + | } | |
| 113 | + | ||
| 114 | + | func (s *Scene) addAsset(blob AssetBlob, copyEncoded bool) (AssetID, error) { | |
| 104 | 115 | if s == nil { | |
| 105 | 116 | return AssetID{}, fmt.Errorf("add asset: %w", ErrInvalidAsset) | |
| 106 | 117 | } | |
| 107 | 118 | s.mu.Lock() | |
| 108 | 119 | defer s.mu.Unlock() | |
| 109 | 120 | candidate := cloneState(s.state) | |
| 110 | - | record, err := s.prepareAsset(blob, candidate) | |
| 121 | + | record, err := s.prepareAsset(blob, candidate, copyEncoded) | |
| 111 | 122 | if err != nil { | |
| 112 | 123 | return AssetID{}, err | |
| 113 | 124 | } | |
| @@ -134,6 +145,16 @@ func (s *Scene) RegisterAsset(blob AssetBlob) (AssetID, error) { return s.AddAss | |||
| 134 | 145 | // reference it. If validation or resource limits reject the new asset, the | |
| 135 | 146 | // existing asset and placements remain unchanged. | |
| 136 | 147 | func (s *Scene) ReplaceAsset(id AssetID, blob AssetBlob) (AssetID, error) { | |
| 148 | + | return s.replaceAsset(id, blob, true) | |
| 149 | + | } | |
| 150 | + | ||
| 151 | + | // ReplaceAssetOwned is ReplaceAsset for callers that hand over blob.Encoded; | |
| 152 | + | // see AddAssetOwned. | |
| 153 | + | func (s *Scene) ReplaceAssetOwned(id AssetID, blob AssetBlob) (AssetID, error) { | |
| 154 | + | return s.replaceAsset(id, blob, false) | |
| 155 | + | } | |
| 156 | + | ||
| 157 | + | func (s *Scene) replaceAsset(id AssetID, blob AssetBlob, copyEncoded bool) (AssetID, error) { | |
| 137 | 158 | if s == nil { | |
| 138 | 159 | return AssetID{}, fmt.Errorf("replace asset: %w", ErrAssetNotFound) | |
| 139 | 160 | } | |
| @@ -155,7 +176,7 @@ func (s *Scene) ReplaceAsset(id AssetID, blob AssetBlob) (AssetID, error) { | |||
| 155 | 176 | candidate.usage.Assets-- | |
| 156 | 177 | candidate.usage.EncodedBytes -= uint64(len(old.encoded)) | |
| 157 | 178 | candidate.usage.DecodedPixels -= old.pixels | |
| 158 | - | replacement, err := s.prepareAsset(blob, candidate) | |
| 179 | + | replacement, err := s.prepareAsset(blob, candidate, copyEncoded) | |
| 159 | 180 | if err != nil { | |
| 160 | 181 | return AssetID{}, err | |
| 161 | 182 | } | |
| @@ -389,7 +410,7 @@ func (s *Scene) Usage() Usage { | |||
| 389 | 410 | return s.state.usage | |
| 390 | 411 | } | |
| 391 | 412 | ||
| 392 | - | func (s *Scene) prepareAsset(blob AssetBlob, state *sceneState) (assetRecord, error) { | |
| 413 | + | func (s *Scene) prepareAsset(blob AssetBlob, state *sceneState, copyEncoded bool) (assetRecord, error) { | |
| 393 | 414 | data := blob.Encoded | |
| 394 | 415 | if blob.Width <= 0 || blob.Height <= 0 { | |
| 395 | 416 | return assetRecord{}, fmt.Errorf("add asset: %w", ErrInvalidAsset) | |
| @@ -414,7 +435,10 @@ func (s *Scene) prepareAsset(blob AssetBlob, state *sceneState) (assetRecord, er | |||
| 414 | 435 | if !within(state.usage.DecodedPixels, pixels, s.limits.MaxDecodedPixels) { | |
| 415 | 436 | return assetRecord{}, fmt.Errorf("add asset: %w", ErrDecodedPixelBudget) | |
| 416 | 437 | } | |
| 417 | - | return assetRecord{encoded: append([]byte(nil), data...), format: blob.Format, width: blob.Width, height: blob.Height, pixels: pixels}, nil | |
| 438 | + | if copyEncoded { | |
| 439 | + | data = append([]byte(nil), data...) | |
| 440 | + | } | |
| 441 | + | return assetRecord{encoded: data, format: blob.Format, width: blob.Width, height: blob.Height, pixels: pixels}, nil | |
| 418 | 442 | } | |
| 419 | 443 | ||
| 420 | 444 | func (s *Scene) preparePlacement(state *sceneState, spec PlacementSpec, existing *placementRecord) (placementRecord, error) { | |
| @@ -495,7 +519,7 @@ func removeAsset(state *sceneState, id AssetID, cascade bool) error { | |||
| 495 | 519 | func (s *Scene) applyOperation(state *sceneState, operation Operation) error { | |
| 496 | 520 | switch operation.Kind { | |
| 497 | 521 | case OperationAddAsset: | |
| 498 | - | record, err := s.prepareAsset(operation.Blob, state) | |
| 522 | + | record, err := s.prepareAsset(operation.Blob, state, true) | |
| 499 | 523 | if err != nil { | |
| 500 | 524 | return err | |
| 501 | 525 | } | |
| @@ -101,13 +101,24 @@ func (s *Scene) Limits() Limits { | |||||
| 101 | 101 | ||||
| 102 | // AddAsset copies and registers an encoded asset, returning an opaque ID. | 102 | // AddAsset copies and registers an encoded asset, returning an opaque ID. | ||
| 103 | func (s *Scene) AddAsset(blob AssetBlob) (AssetID, error) { | 103 | func (s *Scene) AddAsset(blob AssetBlob) (AssetID, error) { | ||
| 104 | + | return s.addAsset(blob, true) | |||
| 105 | + | } | |||
| 106 | + | ||||
| 107 | + | // AddAssetOwned is AddAsset for callers that hand over blob.Encoded: the scene | |||
| 108 | + | // keeps the slice without copying, so the caller must not read or modify it | |||
| 109 | + | // afterwards. Snapshots and AssetView still return copies. | |||
| 110 | + | func (s *Scene) AddAssetOwned(blob AssetBlob) (AssetID, error) { | |||
| 111 | + | return s.addAsset(blob, false) | |||
| 112 | + | } | |||
| 113 | + | ||||
| 114 | + | func (s *Scene) addAsset(blob AssetBlob, copyEncoded bool) (AssetID, error) { | |||
| 104 | if s == nil { | 115 | if s == nil { | ||
| 105 | return AssetID{}, fmt.Errorf("add asset: %w", ErrInvalidAsset) | 116 | return AssetID{}, fmt.Errorf("add asset: %w", ErrInvalidAsset) | ||
| 106 | } | 117 | } | ||
| 107 | s.mu.Lock() | 118 | s.mu.Lock() | ||
| 108 | defer s.mu.Unlock() | 119 | defer s.mu.Unlock() | ||
| 109 | candidate := cloneState(s.state) | 120 | candidate := cloneState(s.state) | ||
| 110 | - | record, err := s.prepareAsset(blob, candidate) | 121 | + | record, err := s.prepareAsset(blob, candidate, copyEncoded) |
| 111 | if err != nil { | 122 | if err != nil { | ||
| 112 | return AssetID{}, err | 123 | return AssetID{}, err | ||
| 113 | } | 124 | } | ||
| @@ -134,6 +145,16 @@ func (s *Scene) RegisterAsset(blob AssetBlob) (AssetID, error) { return s.AddAss | |||||
| 134 | // reference it. If validation or resource limits reject the new asset, the | 145 | // reference it. If validation or resource limits reject the new asset, the | ||
| 135 | // existing asset and placements remain unchanged. | 146 | // existing asset and placements remain unchanged. | ||
| 136 | func (s *Scene) ReplaceAsset(id AssetID, blob AssetBlob) (AssetID, error) { | 147 | func (s *Scene) ReplaceAsset(id AssetID, blob AssetBlob) (AssetID, error) { | ||
| 148 | + | return s.replaceAsset(id, blob, true) | |||
| 149 | + | } | |||
| 150 | + | ||||
| 151 | + | // ReplaceAssetOwned is ReplaceAsset for callers that hand over blob.Encoded; | |||
| 152 | + | // see AddAssetOwned. | |||
| 153 | + | func (s *Scene) ReplaceAssetOwned(id AssetID, blob AssetBlob) (AssetID, error) { | |||
| 154 | + | return s.replaceAsset(id, blob, false) | |||
| 155 | + | } | |||
| 156 | + | ||||
| 157 | + | func (s *Scene) replaceAsset(id AssetID, blob AssetBlob, copyEncoded bool) (AssetID, error) { | |||
| 137 | if s == nil { | 158 | if s == nil { | ||
| 138 | return AssetID{}, fmt.Errorf("replace asset: %w", ErrAssetNotFound) | 159 | return AssetID{}, fmt.Errorf("replace asset: %w", ErrAssetNotFound) | ||
| 139 | } | 160 | } | ||
| @@ -155,7 +176,7 @@ func (s *Scene) ReplaceAsset(id AssetID, blob AssetBlob) (AssetID, error) { | |||||
| 155 | candidate.usage.Assets-- | 176 | candidate.usage.Assets-- | ||
| 156 | candidate.usage.EncodedBytes -= uint64(len(old.encoded)) | 177 | candidate.usage.EncodedBytes -= uint64(len(old.encoded)) | ||
| 157 | candidate.usage.DecodedPixels -= old.pixels | 178 | candidate.usage.DecodedPixels -= old.pixels | ||
| 158 | - | replacement, err := s.prepareAsset(blob, candidate) | 179 | + | replacement, err := s.prepareAsset(blob, candidate, copyEncoded) |
| 159 | if err != nil { | 180 | if err != nil { | ||
| 160 | return AssetID{}, err | 181 | return AssetID{}, err | ||
| 161 | } | 182 | } | ||
| @@ -389,7 +410,7 @@ func (s *Scene) Usage() Usage { | |||||
| 389 | return s.state.usage | 410 | return s.state.usage | ||
| 390 | } | 411 | } | ||
| 391 | 412 | ||||
| 392 | - | func (s *Scene) prepareAsset(blob AssetBlob, state *sceneState) (assetRecord, error) { | 413 | + | func (s *Scene) prepareAsset(blob AssetBlob, state *sceneState, copyEncoded bool) (assetRecord, error) { |
| 393 | data := blob.Encoded | 414 | data := blob.Encoded | ||
| 394 | if blob.Width <= 0 || blob.Height <= 0 { | 415 | if blob.Width <= 0 || blob.Height <= 0 { | ||
| 395 | return assetRecord{}, fmt.Errorf("add asset: %w", ErrInvalidAsset) | 416 | return assetRecord{}, fmt.Errorf("add asset: %w", ErrInvalidAsset) | ||
| @@ -414,7 +435,10 @@ func (s *Scene) prepareAsset(blob AssetBlob, state *sceneState) (assetRecord, er | |||||
| 414 | if !within(state.usage.DecodedPixels, pixels, s.limits.MaxDecodedPixels) { | 435 | if !within(state.usage.DecodedPixels, pixels, s.limits.MaxDecodedPixels) { | ||
| 415 | return assetRecord{}, fmt.Errorf("add asset: %w", ErrDecodedPixelBudget) | 436 | return assetRecord{}, fmt.Errorf("add asset: %w", ErrDecodedPixelBudget) | ||
| 416 | } | 437 | } | ||
| 417 | - | return assetRecord{encoded: append([]byte(nil), data...), format: blob.Format, width: blob.Width, height: blob.Height, pixels: pixels}, nil | 438 | + | if copyEncoded { |
| 439 | + | data = append([]byte(nil), data...) | |||
| 440 | + | } | |||
| 441 | + | return assetRecord{encoded: data, format: blob.Format, width: blob.Width, height: blob.Height, pixels: pixels}, nil | |||
| 418 | } | 442 | } | ||
| 419 | 443 | ||||
| 420 | func (s *Scene) preparePlacement(state *sceneState, spec PlacementSpec, existing *placementRecord) (placementRecord, error) { | 444 | func (s *Scene) preparePlacement(state *sceneState, spec PlacementSpec, existing *placementRecord) (placementRecord, error) { | ||
| @@ -495,7 +519,7 @@ func removeAsset(state *sceneState, id AssetID, cascade bool) error { | |||||
| 495 | func (s *Scene) applyOperation(state *sceneState, operation Operation) error { | 519 | func (s *Scene) applyOperation(state *sceneState, operation Operation) error { | ||
| 496 | switch operation.Kind { | 520 | switch operation.Kind { | ||
| 497 | case OperationAddAsset: | 521 | case OperationAddAsset: | ||
| 498 | - | record, err := s.prepareAsset(operation.Blob, state) | 522 | + | record, err := s.prepareAsset(operation.Blob, state, true) |
| 499 | if err != nil { | 523 | if err != nil { | ||
| 500 | return err | 524 | return err | ||
| 501 | } | 525 | } | ||
M protocol/kittygraphics/base64.go +18 -5
| @@ -1,6 +1,7 @@ | |||
| 1 | 1 | package kittygraphics | |
| 2 | 2 | ||
| 3 | 3 | import ( | |
| 4 | + | "bytes" | |
| 4 | 5 | "encoding/base64" | |
| 5 | 6 | "fmt" | |
| 6 | 7 | ) | |
| @@ -8,14 +9,26 @@ import ( | |||
| 8 | 9 | // DecodeBase64 accepts both padded standard base64 and unpadded raw standard | |
| 9 | 10 | // base64. It deliberately does not accept URL-safe alphabets or non-base64 | |
| 10 | 11 | // whitespace other than the CR/LF ignored by the standard encoding. | |
| 12 | + | // | |
| 13 | + | // The returned slice is freshly allocated and owned by the caller. | |
| 11 | 14 | func DecodeBase64(encoded []byte) ([]byte, error) { | |
| 12 | - | decoded, err := base64.StdEncoding.DecodeString(string(encoded)) | |
| 15 | + | // RawStdEncoding rejects '=' and otherwise accepts every input | |
| 16 | + | // StdEncoding does (padded input has no '=' only when its length is a | |
| 17 | + | // multiple of four, where both encodings agree), so the alphabet used is | |
| 18 | + | // chosen by looking for padding rather than by decoding twice. | |
| 19 | + | enc := base64.RawStdEncoding | |
| 20 | + | if bytes.IndexByte(encoded, '=') >= 0 { | |
| 21 | + | enc = base64.StdEncoding | |
| 22 | + | } | |
| 23 | + | decoded := make([]byte, enc.DecodedLen(len(encoded))) | |
| 24 | + | n, err := enc.Decode(decoded, encoded) | |
| 13 | 25 | if err == nil { | |
| 14 | - | return decoded, nil | |
| 26 | + | return decoded[:n], nil | |
| 15 | 27 | } | |
| 16 | - | decoded, rawErr := base64.RawStdEncoding.DecodeString(string(encoded)) | |
| 17 | - | if rawErr == nil { | |
| 18 | - | return decoded, nil | |
| 28 | + | if enc == base64.RawStdEncoding { | |
| 29 | + | // Report the StdEncoding error, as the padded decoder is the | |
| 30 | + | // documented primary format. This only runs for rejected input. | |
| 31 | + | _, err = base64.StdEncoding.Decode(make([]byte, base64.StdEncoding.DecodedLen(len(encoded))), encoded) | |
| 19 | 32 | } | |
| 20 | 33 | return nil, fmt.Errorf("%w: %v", ErrInvalidBase64, err) | |
| 21 | 34 | } | |
| @@ -1,6 +1,7 @@ | |||||
| 1 | package kittygraphics | 1 | package kittygraphics | ||
| 2 | 2 | ||||
| 3 | import ( | 3 | import ( | ||
| 4 | + | "bytes" | |||
| 4 | "encoding/base64" | 5 | "encoding/base64" | ||
| 5 | "fmt" | 6 | "fmt" | ||
| 6 | ) | 7 | ) | ||
| @@ -8,14 +9,26 @@ import ( | |||||
| 8 | // DecodeBase64 accepts both padded standard base64 and unpadded raw standard | 9 | // DecodeBase64 accepts both padded standard base64 and unpadded raw standard | ||
| 9 | // base64. It deliberately does not accept URL-safe alphabets or non-base64 | 10 | // base64. It deliberately does not accept URL-safe alphabets or non-base64 | ||
| 10 | // whitespace other than the CR/LF ignored by the standard encoding. | 11 | // whitespace other than the CR/LF ignored by the standard encoding. | ||
| 12 | + | // | |||
| 13 | + | // The returned slice is freshly allocated and owned by the caller. | |||
| 11 | func DecodeBase64(encoded []byte) ([]byte, error) { | 14 | func DecodeBase64(encoded []byte) ([]byte, error) { | ||
| 12 | - | decoded, err := base64.StdEncoding.DecodeString(string(encoded)) | 15 | + | // RawStdEncoding rejects '=' and otherwise accepts every input |
| 16 | + | // StdEncoding does (padded input has no '=' only when its length is a | |||
| 17 | + | // multiple of four, where both encodings agree), so the alphabet used is | |||
| 18 | + | // chosen by looking for padding rather than by decoding twice. | |||
| 19 | + | enc := base64.RawStdEncoding | |||
| 20 | + | if bytes.IndexByte(encoded, '=') >= 0 { | |||
| 21 | + | enc = base64.StdEncoding | |||
| 22 | + | } | |||
| 23 | + | decoded := make([]byte, enc.DecodedLen(len(encoded))) | |||
| 24 | + | n, err := enc.Decode(decoded, encoded) | |||
| 13 | if err == nil { | 25 | if err == nil { | ||
| 14 | - | return decoded, nil | 26 | + | return decoded[:n], nil |
| 15 | } | 27 | } | ||
| 16 | - | decoded, rawErr := base64.RawStdEncoding.DecodeString(string(encoded)) | 28 | + | if enc == base64.RawStdEncoding { |
| 17 | - | if rawErr == nil { | 29 | + | // Report the StdEncoding error, as the padded decoder is the |
| 18 | - | return decoded, nil | 30 | + | // documented primary format. This only runs for rejected input. |
| 31 | + | _, err = base64.StdEncoding.Decode(make([]byte, base64.StdEncoding.DecodedLen(len(encoded))), encoded) | |||
| 19 | } | 32 | } | ||
| 20 | return nil, fmt.Errorf("%w: %v", ErrInvalidBase64, err) | 33 | return nil, fmt.Errorf("%w: %v", ErrInvalidBase64, err) | ||
| 21 | } | 34 | } | ||
A protocol/kittygraphics/base64_equiv_test.go +103 -0
| @@ -0,0 +1,103 @@ | |||
| 1 | + | package kittygraphics | |
| 2 | + | ||
| 3 | + | import ( | |
| 4 | + | "bytes" | |
| 5 | + | "encoding/base64" | |
| 6 | + | "fmt" | |
| 7 | + | "math/rand" | |
| 8 | + | "testing" | |
| 9 | + | ) | |
| 10 | + | ||
| 11 | + | // legacyDecodeBase64 is the original two-pass implementation, kept as the | |
| 12 | + | // accept/reject and error-text oracle for DecodeBase64. | |
| 13 | + | func legacyDecodeBase64(encoded []byte) ([]byte, error) { | |
| 14 | + | decoded, err := base64.StdEncoding.DecodeString(string(encoded)) | |
| 15 | + | if err == nil { | |
| 16 | + | return decoded, nil | |
| 17 | + | } | |
| 18 | + | decoded, rawErr := base64.RawStdEncoding.DecodeString(string(encoded)) | |
| 19 | + | if rawErr == nil { | |
| 20 | + | return decoded, nil | |
| 21 | + | } | |
| 22 | + | return nil, fmt.Errorf("%w: %v", ErrInvalidBase64, err) | |
| 23 | + | } | |
| 24 | + | ||
| 25 | + | func checkBase64Equivalent(t testing.TB, input []byte) { | |
| 26 | + | t.Helper() | |
| 27 | + | want, wantErr := legacyDecodeBase64(input) | |
| 28 | + | got, gotErr := DecodeBase64(input) | |
| 29 | + | if (wantErr == nil) != (gotErr == nil) { | |
| 30 | + | t.Fatalf("DecodeBase64(%q) error = %v, legacy error = %v", input, gotErr, wantErr) | |
| 31 | + | } | |
| 32 | + | if wantErr != nil { | |
| 33 | + | if gotErr.Error() != wantErr.Error() { | |
| 34 | + | t.Fatalf("DecodeBase64(%q) error = %q, legacy %q", input, gotErr, wantErr) | |
| 35 | + | } | |
| 36 | + | if !errorsIs(gotErr, ErrInvalidBase64) { | |
| 37 | + | t.Fatalf("DecodeBase64(%q) error %v does not wrap ErrInvalidBase64", input, gotErr) | |
| 38 | + | } | |
| 39 | + | return | |
| 40 | + | } | |
| 41 | + | if !bytes.Equal(got, want) || (got == nil) != (want == nil) { | |
| 42 | + | t.Fatalf("DecodeBase64(%q) = %x, legacy %x", input, got, want) | |
| 43 | + | } | |
| 44 | + | } | |
| 45 | + | ||
| 46 | + | func TestDecodeBase64MatchesLegacyRandom(t *testing.T) { | |
| 47 | + | rng := rand.New(rand.NewSource(1)) | |
| 48 | + | alphabets := []string{ | |
| 49 | + | "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/", | |
| 50 | + | "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/=\r\n", | |
| 51 | + | "AAAA==\r\n=-_ \t\x00\xff", | |
| 52 | + | } | |
| 53 | + | for i := 0; i < 200000; i++ { | |
| 54 | + | alphabet := alphabets[rng.Intn(len(alphabets))] | |
| 55 | + | input := make([]byte, rng.Intn(24)) | |
| 56 | + | for j := range input { | |
| 57 | + | input[j] = alphabet[rng.Intn(len(alphabet))] | |
| 58 | + | } | |
| 59 | + | checkBase64Equivalent(t, input) | |
| 60 | + | } | |
| 61 | + | // Valid encodings, padded and raw, with and without line breaks. | |
| 62 | + | for n := 0; n < 40; n++ { | |
| 63 | + | raw := make([]byte, n) | |
| 64 | + | rng.Read(raw) | |
| 65 | + | padded := base64.StdEncoding.EncodeToString(raw) | |
| 66 | + | unpadded := base64.RawStdEncoding.EncodeToString(raw) | |
| 67 | + | checkBase64Equivalent(t, []byte(padded)) | |
| 68 | + | checkBase64Equivalent(t, []byte(unpadded)) | |
| 69 | + | checkBase64Equivalent(t, []byte(unpadded+"\r\n")) | |
| 70 | + | if len(padded) > 2 { | |
| 71 | + | checkBase64Equivalent(t, []byte(padded[:2]+"\n"+padded[2:])) | |
| 72 | + | checkBase64Equivalent(t, []byte(padded[:len(padded)-1])) | |
| 73 | + | } | |
| 74 | + | } | |
| 75 | + | checkBase64Equivalent(t, nil) | |
| 76 | + | checkBase64Equivalent(t, []byte{}) | |
| 77 | + | } | |
| 78 | + | ||
| 79 | + | func FuzzDecodeBase64MatchesLegacy(f *testing.F) { | |
| 80 | + | for _, seed := range []string{"", "AAAA", "AAA=", "AAA", "AA==", "AA", "A", "AA=", "=", "AAAA\n", "AA\r\nAA", "-_-_"} { | |
| 81 | + | f.Add([]byte(seed)) | |
| 82 | + | } | |
| 83 | + | f.Fuzz(func(t *testing.T, input []byte) { checkBase64Equivalent(t, input) }) | |
| 84 | + | } | |
| 85 | + | ||
| 86 | + | func TestParseAPCBorrowedAliasesAndParseAPCCopies(t *testing.T) { | |
| 87 | + | apc := []byte("\x1b_Ga=t,i=1;QUJD\x1b\\") | |
| 88 | + | copied, err := ParseAPC(apc) | |
| 89 | + | if err != nil { | |
| 90 | + | t.Fatal(err) | |
| 91 | + | } | |
| 92 | + | borrowed, err := ParseAPCBorrowed(apc) | |
| 93 | + | if err != nil { | |
| 94 | + | t.Fatal(err) | |
| 95 | + | } | |
| 96 | + | apc[len("\x1b_Ga=t,i=1;")] = 'Z' | |
| 97 | + | if string(copied.Payload) != "QUJD" { | |
| 98 | + | t.Fatalf("ParseAPC payload aliased input: %q", copied.Payload) | |
| 99 | + | } | |
| 100 | + | if string(borrowed.Payload) != "ZUJD" { | |
| 101 | + | t.Fatalf("ParseAPCBorrowed payload = %q, want aliased", borrowed.Payload) | |
| 102 | + | } | |
| 103 | + | } | |
| @@ -0,0 +1,103 @@ | |||||
| 1 | + | package kittygraphics | |||
| 2 | + | ||||
| 3 | + | import ( | |||
| 4 | + | "bytes" | |||
| 5 | + | "encoding/base64" | |||
| 6 | + | "fmt" | |||
| 7 | + | "math/rand" | |||
| 8 | + | "testing" | |||
| 9 | + | ) | |||
| 10 | + | ||||
| 11 | + | // legacyDecodeBase64 is the original two-pass implementation, kept as the | |||
| 12 | + | // accept/reject and error-text oracle for DecodeBase64. | |||
| 13 | + | func legacyDecodeBase64(encoded []byte) ([]byte, error) { | |||
| 14 | + | decoded, err := base64.StdEncoding.DecodeString(string(encoded)) | |||
| 15 | + | if err == nil { | |||
| 16 | + | return decoded, nil | |||
| 17 | + | } | |||
| 18 | + | decoded, rawErr := base64.RawStdEncoding.DecodeString(string(encoded)) | |||
| 19 | + | if rawErr == nil { | |||
| 20 | + | return decoded, nil | |||
| 21 | + | } | |||
| 22 | + | return nil, fmt.Errorf("%w: %v", ErrInvalidBase64, err) | |||
| 23 | + | } | |||
| 24 | + | ||||
| 25 | + | func checkBase64Equivalent(t testing.TB, input []byte) { | |||
| 26 | + | t.Helper() | |||
| 27 | + | want, wantErr := legacyDecodeBase64(input) | |||
| 28 | + | got, gotErr := DecodeBase64(input) | |||
| 29 | + | if (wantErr == nil) != (gotErr == nil) { | |||
| 30 | + | t.Fatalf("DecodeBase64(%q) error = %v, legacy error = %v", input, gotErr, wantErr) | |||
| 31 | + | } | |||
| 32 | + | if wantErr != nil { | |||
| 33 | + | if gotErr.Error() != wantErr.Error() { | |||
| 34 | + | t.Fatalf("DecodeBase64(%q) error = %q, legacy %q", input, gotErr, wantErr) | |||
| 35 | + | } | |||
| 36 | + | if !errorsIs(gotErr, ErrInvalidBase64) { | |||
| 37 | + | t.Fatalf("DecodeBase64(%q) error %v does not wrap ErrInvalidBase64", input, gotErr) | |||
| 38 | + | } | |||
| 39 | + | return | |||
| 40 | + | } | |||
| 41 | + | if !bytes.Equal(got, want) || (got == nil) != (want == nil) { | |||
| 42 | + | t.Fatalf("DecodeBase64(%q) = %x, legacy %x", input, got, want) | |||
| 43 | + | } | |||
| 44 | + | } | |||
| 45 | + | ||||
| 46 | + | func TestDecodeBase64MatchesLegacyRandom(t *testing.T) { | |||
| 47 | + | rng := rand.New(rand.NewSource(1)) | |||
| 48 | + | alphabets := []string{ | |||
| 49 | + | "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/", | |||
| 50 | + | "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/=\r\n", | |||
| 51 | + | "AAAA==\r\n=-_ \t\x00\xff", | |||
| 52 | + | } | |||
| 53 | + | for i := 0; i < 200000; i++ { | |||
| 54 | + | alphabet := alphabets[rng.Intn(len(alphabets))] | |||
| 55 | + | input := make([]byte, rng.Intn(24)) | |||
| 56 | + | for j := range input { | |||
| 57 | + | input[j] = alphabet[rng.Intn(len(alphabet))] | |||
| 58 | + | } | |||
| 59 | + | checkBase64Equivalent(t, input) | |||
| 60 | + | } | |||
| 61 | + | // Valid encodings, padded and raw, with and without line breaks. | |||
| 62 | + | for n := 0; n < 40; n++ { | |||
| 63 | + | raw := make([]byte, n) | |||
| 64 | + | rng.Read(raw) | |||
| 65 | + | padded := base64.StdEncoding.EncodeToString(raw) | |||
| 66 | + | unpadded := base64.RawStdEncoding.EncodeToString(raw) | |||
| 67 | + | checkBase64Equivalent(t, []byte(padded)) | |||
| 68 | + | checkBase64Equivalent(t, []byte(unpadded)) | |||
| 69 | + | checkBase64Equivalent(t, []byte(unpadded+"\r\n")) | |||
| 70 | + | if len(padded) > 2 { | |||
| 71 | + | checkBase64Equivalent(t, []byte(padded[:2]+"\n"+padded[2:])) | |||
| 72 | + | checkBase64Equivalent(t, []byte(padded[:len(padded)-1])) | |||
| 73 | + | } | |||
| 74 | + | } | |||
| 75 | + | checkBase64Equivalent(t, nil) | |||
| 76 | + | checkBase64Equivalent(t, []byte{}) | |||
| 77 | + | } | |||
| 78 | + | ||||
| 79 | + | func FuzzDecodeBase64MatchesLegacy(f *testing.F) { | |||
| 80 | + | for _, seed := range []string{"", "AAAA", "AAA=", "AAA", "AA==", "AA", "A", "AA=", "=", "AAAA\n", "AA\r\nAA", "-_-_"} { | |||
| 81 | + | f.Add([]byte(seed)) | |||
| 82 | + | } | |||
| 83 | + | f.Fuzz(func(t *testing.T, input []byte) { checkBase64Equivalent(t, input) }) | |||
| 84 | + | } | |||
| 85 | + | ||||
| 86 | + | func TestParseAPCBorrowedAliasesAndParseAPCCopies(t *testing.T) { | |||
| 87 | + | apc := []byte("\x1b_Ga=t,i=1;QUJD\x1b\\") | |||
| 88 | + | copied, err := ParseAPC(apc) | |||
| 89 | + | if err != nil { | |||
| 90 | + | t.Fatal(err) | |||
| 91 | + | } | |||
| 92 | + | borrowed, err := ParseAPCBorrowed(apc) | |||
| 93 | + | if err != nil { | |||
| 94 | + | t.Fatal(err) | |||
| 95 | + | } | |||
| 96 | + | apc[len("\x1b_Ga=t,i=1;")] = 'Z' | |||
| 97 | + | if string(copied.Payload) != "QUJD" { | |||
| 98 | + | t.Fatalf("ParseAPC payload aliased input: %q", copied.Payload) | |||
| 99 | + | } | |||
| 100 | + | if string(borrowed.Payload) != "ZUJD" { | |||
| 101 | + | t.Fatalf("ParseAPCBorrowed payload = %q, want aliased", borrowed.Payload) | |||
| 102 | + | } | |||
| 103 | + | } | |||
M protocol/kittygraphics/parser.go +22 -2
| @@ -159,6 +159,10 @@ func (p *Parser) commandEvent() Event { | |||
| 159 | 159 | // exported separately so adapters can validate commands without maintaining a | |
| 160 | 160 | // stream parser. | |
| 161 | 161 | func ParseCommand(body []byte, config ...Limits) (Command, error) { | |
| 162 | + | return parseCommand(body, true, config) | |
| 163 | + | } | |
| 164 | + | ||
| 165 | + | func parseCommand(body []byte, copyPayload bool, config []Limits) (Command, error) { | |
| 162 | 166 | var l Limits | |
| 163 | 167 | if len(config) != 0 { | |
| 164 | 168 | l = config[0] | |
| @@ -179,11 +183,27 @@ func ParseCommand(body []byte, config ...Limits) (Command, error) { | |||
| 179 | 183 | if uint64(len(payload)) > l.MaxPayloadBytes { | |
| 180 | 184 | return Command{}, ErrPayloadTooLarge | |
| 181 | 185 | } | |
| 182 | - | return Command{Controls: controls, Payload: append([]byte(nil), payload...)}, nil | |
| 186 | + | if copyPayload { | |
| 187 | + | payload = append([]byte(nil), payload...) | |
| 188 | + | } | |
| 189 | + | return Command{Controls: controls, Payload: payload}, nil | |
| 183 | 190 | } | |
| 184 | 191 | ||
| 185 | 192 | // ParseAPC parses a complete ESC _ G ... ST sequence. | |
| 186 | 193 | func ParseAPC(apc []byte, config ...Limits) (Command, error) { | |
| 194 | + | return parseAPC(apc, true, config) | |
| 195 | + | } | |
| 196 | + | ||
| 197 | + | // ParseAPCBorrowed is ParseAPC without copying the payload: the returned | |
| 198 | + | // Command.Payload aliases apc. The caller must not modify apc while the | |
| 199 | + | // command is in use and must not retain the command beyond the lifetime of | |
| 200 | + | // apc. Session.Process copies whatever it keeps from a command's payload, so | |
| 201 | + | // a borrowed command may be passed to it and apc reused afterwards. | |
| 202 | + | func ParseAPCBorrowed(apc []byte, config ...Limits) (Command, error) { | |
| 203 | + | return parseAPC(apc, false, config) | |
| 204 | + | } | |
| 205 | + | ||
| 206 | + | func parseAPC(apc []byte, copyPayload bool, config []Limits) (Command, error) { | |
| 187 | 207 | if len(apc) < 4 || apc[0] != 0x1b || apc[1] != '_' || apc[2] != 'G' { | |
| 188 | 208 | return Command{}, ErrInvalidAPC | |
| 189 | 209 | } | |
| @@ -195,5 +215,5 @@ func ParseAPC(apc []byte, config ...Limits) (Command, error) { | |||
| 195 | 215 | } else { | |
| 196 | 216 | return Command{}, ErrAPCTruncated | |
| 197 | 217 | } | |
| 198 | - | return ParseCommand(body, config...) | |
| 218 | + | return parseCommand(body, copyPayload, config) | |
| 199 | 219 | } | |
| @@ -159,6 +159,10 @@ func (p *Parser) commandEvent() Event { | |||||
| 159 | // exported separately so adapters can validate commands without maintaining a | 159 | // exported separately so adapters can validate commands without maintaining a | ||
| 160 | // stream parser. | 160 | // stream parser. | ||
| 161 | func ParseCommand(body []byte, config ...Limits) (Command, error) { | 161 | func ParseCommand(body []byte, config ...Limits) (Command, error) { | ||
| 162 | + | return parseCommand(body, true, config) | |||
| 163 | + | } | |||
| 164 | + | ||||
| 165 | + | func parseCommand(body []byte, copyPayload bool, config []Limits) (Command, error) { | |||
| 162 | var l Limits | 166 | var l Limits | ||
| 163 | if len(config) != 0 { | 167 | if len(config) != 0 { | ||
| 164 | l = config[0] | 168 | l = config[0] | ||
| @@ -179,11 +183,27 @@ func ParseCommand(body []byte, config ...Limits) (Command, error) { | |||||
| 179 | if uint64(len(payload)) > l.MaxPayloadBytes { | 183 | if uint64(len(payload)) > l.MaxPayloadBytes { | ||
| 180 | return Command{}, ErrPayloadTooLarge | 184 | return Command{}, ErrPayloadTooLarge | ||
| 181 | } | 185 | } | ||
| 182 | - | return Command{Controls: controls, Payload: append([]byte(nil), payload...)}, nil | 186 | + | if copyPayload { |
| 187 | + | payload = append([]byte(nil), payload...) | |||
| 188 | + | } | |||
| 189 | + | return Command{Controls: controls, Payload: payload}, nil | |||
| 183 | } | 190 | } | ||
| 184 | 191 | ||||
| 185 | // ParseAPC parses a complete ESC _ G ... ST sequence. | 192 | // ParseAPC parses a complete ESC _ G ... ST sequence. | ||
| 186 | func ParseAPC(apc []byte, config ...Limits) (Command, error) { | 193 | func ParseAPC(apc []byte, config ...Limits) (Command, error) { | ||
| 194 | + | return parseAPC(apc, true, config) | |||
| 195 | + | } | |||
| 196 | + | ||||
| 197 | + | // ParseAPCBorrowed is ParseAPC without copying the payload: the returned | |||
| 198 | + | // Command.Payload aliases apc. The caller must not modify apc while the | |||
| 199 | + | // command is in use and must not retain the command beyond the lifetime of | |||
| 200 | + | // apc. Session.Process copies whatever it keeps from a command's payload, so | |||
| 201 | + | // a borrowed command may be passed to it and apc reused afterwards. | |||
| 202 | + | func ParseAPCBorrowed(apc []byte, config ...Limits) (Command, error) { | |||
| 203 | + | return parseAPC(apc, false, config) | |||
| 204 | + | } | |||
| 205 | + | ||||
| 206 | + | func parseAPC(apc []byte, copyPayload bool, config []Limits) (Command, error) { | |||
| 187 | if len(apc) < 4 || apc[0] != 0x1b || apc[1] != '_' || apc[2] != 'G' { | 207 | if len(apc) < 4 || apc[0] != 0x1b || apc[1] != '_' || apc[2] != 'G' { | ||
| 188 | return Command{}, ErrInvalidAPC | 208 | return Command{}, ErrInvalidAPC | ||
| 189 | } | 209 | } | ||
| @@ -195,5 +215,5 @@ func ParseAPC(apc []byte, config ...Limits) (Command, error) { | |||||
| 195 | } else { | 215 | } else { | ||
| 196 | return Command{}, ErrAPCTruncated | 216 | return Command{}, ErrAPCTruncated | ||
| 197 | } | 217 | } | ||
| 198 | - | return ParseCommand(body, config...) | 218 | + | return parseCommand(body, copyPayload, config) |
| 199 | } | 219 | } | ||
M protocol/kittygraphics/session.go +9 -7
| @@ -287,14 +287,16 @@ func (s *Session) transmit(command Command, display bool) ([][]byte, *Mutation, | |||
| 287 | 287 | s.upload = &upload{ | |
| 288 | 288 | controls: c, | |
| 289 | 289 | imageID: imageID, | |
| 290 | - | payload: append([]byte(nil), command.Payload...), | |
| 291 | - | chunks: 1, | |
| 292 | - | display: display, | |
| 293 | - | origin: s.origin, | |
| 290 | + | // The session keeps the payload across Process calls, so it must | |
| 291 | + | // not alias the caller's (possibly borrowed) bytes. | |
| 292 | + | payload: append([]byte(nil), command.Payload...), | |
| 293 | + | chunks: 1, | |
| 294 | + | display: display, | |
| 295 | + | origin: s.origin, | |
| 294 | 296 | } | |
| 295 | 297 | return nil, nil, nil | |
| 296 | 298 | } | |
| 297 | - | return s.commitUpload(upload{controls: c, imageID: imageID, payload: append([]byte(nil), command.Payload...), chunks: 1, display: display, origin: s.origin}, display) | |
| 299 | + | return s.commitUpload(upload{controls: c, imageID: imageID, payload: command.Payload, chunks: 1, display: display, origin: s.origin}, display) | |
| 298 | 300 | } | |
| 299 | 301 | ||
| 300 | 302 | func (s *Session) commitUpload(value upload, display bool) ([][]byte, *Mutation, error) { | |
| @@ -336,14 +338,14 @@ func (s *Session) commitUpload(value upload, display bool) ([][]byte, *Mutation, | |||
| 336 | 338 | if replacing { | |
| 337 | 339 | // Scene replacement is copy-on-write: a rejected new asset leaves the | |
| 338 | 340 | // old asset and every placement that references it untouched. | |
| 339 | - | assetID, err = s.scene.ReplaceAsset(old, graphics.AssetBlob{ | |
| 341 | + | assetID, err = s.scene.ReplaceAssetOwned(old, graphics.AssetBlob{ | |
| 340 | 342 | Encoded: decoded, | |
| 341 | 343 | Format: assetFormat(format), | |
| 342 | 344 | Width: width, | |
| 343 | 345 | Height: height, | |
| 344 | 346 | }) | |
| 345 | 347 | } else { | |
| 346 | - | assetID, err = s.scene.AddAsset(graphics.AssetBlob{ | |
| 348 | + | assetID, err = s.scene.AddAssetOwned(graphics.AssetBlob{ | |
| 347 | 349 | Encoded: decoded, | |
| 348 | 350 | Format: assetFormat(format), | |
| 349 | 351 | Width: width, | |
| @@ -287,14 +287,16 @@ func (s *Session) transmit(command Command, display bool) ([][]byte, *Mutation, | |||||
| 287 | s.upload = &upload{ | 287 | s.upload = &upload{ | ||
| 288 | controls: c, | 288 | controls: c, | ||
| 289 | imageID: imageID, | 289 | imageID: imageID, | ||
| 290 | - | payload: append([]byte(nil), command.Payload...), | 290 | + | // The session keeps the payload across Process calls, so it must |
| 291 | - | chunks: 1, | 291 | + | // not alias the caller's (possibly borrowed) bytes. |
| 292 | - | display: display, | 292 | + | payload: append([]byte(nil), command.Payload...), |
| 293 | - | origin: s.origin, | 293 | + | chunks: 1, |
| 294 | + | display: display, | |||
| 295 | + | origin: s.origin, | |||
| 294 | } | 296 | } | ||
| 295 | return nil, nil, nil | 297 | return nil, nil, nil | ||
| 296 | } | 298 | } | ||
| 297 | - | return s.commitUpload(upload{controls: c, imageID: imageID, payload: append([]byte(nil), command.Payload...), chunks: 1, display: display, origin: s.origin}, display) | 299 | + | return s.commitUpload(upload{controls: c, imageID: imageID, payload: command.Payload, chunks: 1, display: display, origin: s.origin}, display) |
| 298 | } | 300 | } | ||
| 299 | 301 | ||||
| 300 | func (s *Session) commitUpload(value upload, display bool) ([][]byte, *Mutation, error) { | 302 | func (s *Session) commitUpload(value upload, display bool) ([][]byte, *Mutation, error) { | ||
| @@ -336,14 +338,14 @@ func (s *Session) commitUpload(value upload, display bool) ([][]byte, *Mutation, | |||||
| 336 | if replacing { | 338 | if replacing { | ||
| 337 | // Scene replacement is copy-on-write: a rejected new asset leaves the | 339 | // Scene replacement is copy-on-write: a rejected new asset leaves the | ||
| 338 | // old asset and every placement that references it untouched. | 340 | // old asset and every placement that references it untouched. | ||
| 339 | - | assetID, err = s.scene.ReplaceAsset(old, graphics.AssetBlob{ | 341 | + | assetID, err = s.scene.ReplaceAssetOwned(old, graphics.AssetBlob{ |
| 340 | Encoded: decoded, | 342 | Encoded: decoded, | ||
| 341 | Format: assetFormat(format), | 343 | Format: assetFormat(format), | ||
| 342 | Width: width, | 344 | Width: width, | ||
| 343 | Height: height, | 345 | Height: height, | ||
| 344 | }) | 346 | }) | ||
| 345 | } else { | 347 | } else { | ||
| 346 | - | assetID, err = s.scene.AddAsset(graphics.AssetBlob{ | 348 | + | assetID, err = s.scene.AddAssetOwned(graphics.AssetBlob{ |
| 347 | Encoded: decoded, | 349 | Encoded: decoded, | ||
| 348 | Format: assetFormat(format), | 350 | Format: assetFormat(format), | ||
| 349 | Width: width, | 351 | Width: width, | ||