Promote SKPaint text/font obsolete members to error#4068
Conversation
Phase 1 of mono#3732. Promotes the warning-only [Obsolete] attributes on SKPaint's ~115 text/font members, the 5 paint-only DrawText/DrawTextOnPath overloads on SKCanvas, and the 11 SKFilterQuality-consuming overloads across SKBitmap/SKImage/SKShader/SKPixmap to [Obsolete(..., error: true)], so they are stripped from the reference assembly by the existing RemoveObsoleteSymbols build task. Internal callers in SKCanvas (DrawImage/DrawAtlas/DrawText) and SkiaSharp.HarfBuzz continue to respect legacy paint.FilterQuality and paint.TextAlign state through four new non-obsolete bypass helpers on SKPaint (GetLegacyFont, GetLegacyTextAlign, GetLegacyTextEncoding, GetLegacyFilterQualitySampling), exposed via the existing InternalsVisibleTo declarations. No behavior change visible to consumers of either binding. Also incorporates the two warning-level DrawText obsoletions from PR mono#3165, with affected Gallery samples migrated to pass SKTextAlign.Left explicitly (none of them set paint.TextAlign). Obsolete SKPaint text-API tests removed from SKPaintTest.cs; equivalent SKFont-API coverage already exists in SKFontTest.cs. SkCompatPaint stays in the native shim per the issue's Phase 1 plan. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📦 Try the packages from this PRWarning Do not run these scripts without first reviewing the code in this PR. Step 1 — Download the packages bash / macOS / Linux: curl -fsSL https://raw.githubusercontent.com/mono/SkiaSharp/main/scripts/get-skiasharp-pr.sh | bash -s -- 4068PowerShell / Windows: iex "& { $(irm https://raw.githubusercontent.com/mono/SkiaSharp/main/scripts/get-skiasharp-pr.ps1) } 4068"Step 2 — Add the local NuGet source dotnet nuget add source ~/.skiasharp/hives/pr-4068/packages --name skiasharp-pr-4068More options
Or download manually from Azure Pipelines — look for the Remove the source when you're done: dotnet nuget remove source skiasharp-pr-4068 |
|
/azp run |
|
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. |
|
No pipelines are associated with this pull request. |
SKImageInfo.BytesSize (Width * Height * BytesPerPixel) and RowBytes (Width * BytesPerPixel) were computed in int32 with no overflow check. With attacker-influenced dimensions, the product silently wraps. The classic exploit pattern - e.g. Width=Height=65536, BytesPerPixel=4 - gives a real size of 17 GB and an int32 result of 0, so new byte[info.BytesSize] allocates an empty array that the native decoder then writes ~17 GB into. Heap corruption. Affected sinks reachable from public API: - SKImage.Create(SKImageInfo): Marshal.AllocCoTaskMem(info.BytesSize) - SKCodec.GetPixels(info, out byte[]): new byte[info.BytesSize] passed to native decoder (attacker controls info via image header) - SKBitmap.Pixels getter/setter: new SKColor[Width * Height] and the Width * Height bounds check (the check itself overflows) - SKPixmap.GetPixelSpan<T>: Span<T> sized smaller than the underlying native buffer Fix: wrap BytesSize, RowBytes, GetPixelBytesOffset, the SKBitmap.Pixels arithmetic, and SKPixmap.GetPixelSpan<T> length with checked() so overflow throws OverflowException instead of corrupting memory. Widen the SKBitmap.Pixels setter's bounds check to 64-bit math so the guard can't be defeated. The existing BytesSize64 / RowBytes64 long-cast variants remain available for code that needs to inspect oversized dimensions without throwing. Adds three regression tests in SKImageInfoTest covering the wrap-to- zero case, the RowBytes wrap case, and the just-under-int.MaxValue boundary that must still succeed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No pipelines are associated with this pull request. |
There was a problem hiding this comment.
Pull request overview
This PR is Phase 1 of #3732, promoting legacy SKPaint text/font APIs (and several SKFilterQuality-based overloads) from warning-only [Obsolete] to [Obsolete(..., error: true)] so they can be stripped from the reference assembly, while keeping internal behavior compatible via new legacy-bypass helpers.
Changes:
- Promote SKPaint text/font-related obsolete members (plus selected SKCanvas DrawText/DrawTextOnPath and SKFilterQuality overloads) to
error: true. - Add internal “legacy” accessors on
SKPaintand update SkiaSharp.HarfBuzz + core drawing paths to preserve legacy state without referencing error-obsolete members from non-obsolete APIs. - Add overflow-guarding (
checked) to size/stride computations and extend tests; migrate Gallery samples to passSKTextAlign.Leftexplicitly.
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Tests/SkiaSharp/SKPaintTest.cs | Removes obsolete SKPaint text API tests (relying on SKFont coverage elsewhere). |
| tests/Tests/SkiaSharp/SKBasicTypesTest.cs | Adds tests ensuring SKImageInfo.BytesSize/RowBytes throw on int overflow. |
| source/SkiaSharp.HarfBuzz/SkiaSharp.HarfBuzz/SKShaper.cs | Switches legacy paint access to new GetLegacy* helpers. |
| source/SkiaSharp.HarfBuzz/SkiaSharp.HarfBuzz/CanvasExtensions.cs | Removes direct use of obsolete paint state; uses legacy helpers instead. |
| samples/Gallery/Shared/Samples/WorldTextSample.cs | Updates DrawText calls to pass SKTextAlign.Left. |
| samples/Gallery/Shared/Samples/WideGamutP3Sample.cs | Updates DrawText calls to pass SKTextAlign.Left. |
| samples/Gallery/Shared/Samples/VariableFontSample.cs | Updates DrawText calls to pass SKTextAlign.Left. |
| samples/Gallery/Shared/Samples/ThreeDSample.cs | Updates DrawText calls to pass SKTextAlign.Left. |
| samples/Gallery/Shared/Samples/TextToPathSample.cs | Updates DrawText calls to pass SKTextAlign.Left. |
| samples/Gallery/Shared/Samples/TextOnPathSample.cs | Updates DrawText calls to pass SKTextAlign.Left. |
| samples/Gallery/Shared/Samples/TextLabSample.cs | Updates DrawText calls to pass SKTextAlign.Left. |
| samples/Gallery/Shared/Samples/SmartTextUnderlineSample.cs | Updates DrawText calls to pass SKTextAlign.Left. |
| samples/Gallery/Shared/Samples/ShaderCrossFadeSample.cs | Updates DrawText calls to pass SKTextAlign.Left. |
| samples/Gallery/Shared/Samples/PerlinNoiseTexturesSample.cs | Updates DrawText calls to pass SKTextAlign.Left. |
| samples/Gallery/Shared/Samples/PdfComposerSample.cs | Updates DrawText calls to pass SKTextAlign.Left. |
| samples/Gallery/Shared/Samples/PathEffectsSamplerSample.cs | Updates DrawText calls to pass SKTextAlign.Left. |
| samples/Gallery/Shared/Samples/ImageDecoderSample.cs | Updates DrawText calls to pass SKTextAlign.Left. |
| samples/Gallery/Shared/Samples/HeroImageSample.cs | Updates DrawText calls to pass SKTextAlign.Left. |
| samples/Gallery/Shared/Samples/GifPlayerSample.cs | Updates DrawText calls to pass SKTextAlign.Left. |
| samples/Gallery/Shared/Samples/ColorFontSample.cs | Updates DrawText calls to pass SKTextAlign.Left. |
| samples/Gallery/Shared/Samples/CanvasTransformsSample.cs | Updates DrawText calls to pass SKTextAlign.Left. |
| samples/Gallery/Shared/Samples/AnimatedWebpEncoderSample.cs | Updates DrawText calls to pass SKTextAlign.Left. |
| binding/SkiaSharp/SKShader.cs | Marks SKFilterQuality overloads as error: true. |
| binding/SkiaSharp/SKPixmap.cs | Adds checked math; marks SKFilterQuality overload as error: true. |
| binding/SkiaSharp/SKPaint.cs | Promotes many text/font-related members to error: true; adds internal legacy-bypass helpers. |
| binding/SkiaSharp/SKImageInfo.cs | Adds checked overflow guards to size/stride calculations. |
| binding/SkiaSharp/SKImage.cs | Marks SKFilterQuality overloads as error: true. |
| binding/SkiaSharp/SKCanvas.cs | Uses legacy filter-quality/text-align helpers in non-obsolete overload paths. |
| binding/SkiaSharp/SKBitmap.cs | Adds checked for pixel counts; marks SKFilterQuality overloads as error: true. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
No pipelines are associated with this pull request. |
…te-skpaint-text-errors # Conflicts: # tests/Tests/SkiaSharp/SKPaintTest.cs
|
No pipelines are associated with this pull request. |
The SkipOnPlatform(IsBrowser, ...) line was accidentally removed during the bulk deletion of obsolete test methods, causing WASM CI to fail. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
No pipelines are associated with this pull request. |
These overloads implicitly read paint.TextAlign which is itself obsolete. Promote them to error: true and update all call sites to use the explicit SKTextAlign.Left overload. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The DrawText(text, x, y, font, paint) overload is now [Obsolete(error: true)]. Migrate to the overload that takes SKTextAlign.Left explicitly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
No pipelines are associated with this pull request. |
Obsolete remaining legacy paint state-reading APIs (#4114) Context: #3732 Context: #4113 Builds on: #4068 Several public APIs on SKCanvas, SKTypeface, and SkiaSharp.HarfBuzz silently read legacy state from SKPaint (FilterQuality, TextAlign, or the embedded SKFont) rather than accepting explicit parameters. Consumers cannot tell from the signature that behavior depends on mutable paint state, and the implicit reads block eventual removal of the compat-paint shim in Phase 2 of #3732. This marks those APIs obsolete following the same two-tier rule used in #4068: * Already obsolete on main → promote to [Obsolete("…", error: true)] * Newly obsolete (not previously marked) → [Obsolete("…")] warning SKPaint internals: * GetFont() narrowed from internal to private — only reachable from already-obsolete members. * All four GetLegacy* bridge methods marked [Obsolete]. The compiler auto-suppresses CS0618 when the caller is itself obsolete, so no pragma directives are needed anywhere. SKCanvas: * 4 DrawImage, 4 DrawBitmap, 3 DrawAtlas overloads (read FilterQuality) and 3 DrawTextOnPath overloads (read TextAlign) marked obsolete. * 4 new DrawBitmap(..., SKSamplingOptions, SKPaint) overloads added as the migration target — callers add one parameter instead of converting bitmaps to images. SKTypeface: * 12 public glyph-query methods (CountGlyphs, GetGlyph, GetGlyphs, ContainsGlyph, ContainsGlyphs variants) marked obsolete. These internally create a throw-away SKFont; callers should use SKFont directly. Equivalent test coverage already exists in SKFontTest. HarfBuzz: * 4 paint-only DrawShapedText and 4 paint-only Shape overloads promoted to error-level obsolete. * 4 font+paint DrawShapedText overloads (read paint.TextAlign) marked as warnings pointing to the SKTextAlign overloads. Gallery samples and tests migrated to non-obsolete overloads throughout. Tests that specifically exercise obsolete SKTypeface glyph APIs are marked [Obsolete] themselves rather than suppressed with pragmas. Co-authored-by: Matthew Leibowitz <mattleibow@live.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Description of Change
Phase 1 of #3732. Promotes the warning-only [Obsolete] attributes on SKPaint's ~115 text/font members, the 5 paint-only DrawText/DrawTextOnPath overloads on SKCanvas, and the 11 SKFilterQuality-consuming overloads across SKBitmap/SKImage/SKShader/SKPixmap to [Obsolete(..., error: true)], so they are stripped from the reference assembly by the existing RemoveObsoleteSymbols build task.
Internal callers in SKCanvas (DrawImage/DrawAtlas/DrawText) and SkiaSharp.HarfBuzz continue to respect legacy paint.FilterQuality and paint.TextAlign state through four new non-obsolete bypass helpers on SKPaint (GetLegacyFont, GetLegacyTextAlign, GetLegacyTextEncoding, GetLegacyFilterQualitySampling), exposed via the existing InternalsVisibleTo declarations. No behavior change visible to consumers of either binding.
Also incorporates the two warning-level DrawText obsoletions from PR #3165, with affected Gallery samples migrated to pass SKTextAlign.Left explicitly (none of them set paint.TextAlign).
Obsolete SKPaint text-API tests removed from SKPaintTest.cs; equivalent SKFont-API coverage already exists in SKFontTest.cs. SkCompatPaint stays in the native shim per the issue's Phase 1 plan.
Bugs Fixed
API Changes
None.
Behavioral Changes
None.
Required skia PR
None.
PR Checklist