mono / SkiaSharp

SkiaSharp is a cross-platform 2D graphics API for .NET platforms based on Google's Skia Graphics Library. It provides a comprehensive 2D API that can be used across mobile, server and desktop models to render images.
MIT License
4.14k stars 522 forks source link

[BUG] `SKNativeObject` instances are not kept alive during native API calls #2821

Open kpreisser opened 1 month ago

kpreisser commented 1 month ago

Description

Hi there, I'm currently integrating SkiaSharp in a project, and it seems to work well so far. However, when looking through the source code, unless I'm missing something, I think there's an issue/bug in how the native handle of SKNativeObject (e.g. SKPaint) is freed by the finalizer (i.e. when you don't explicitly call Dispose()).

Generally, when a managed object wraps a native handle/pointer, you want to ensure that the handle is not released "prematurely", i.e. when it is still in use. See the following comment in GC.KeepAlive():

// The JIT is very aggressive about keeping an // object's lifetime to as small a window as possible, to the point // where a 'this' pointer isn't considered live in an instance method // unless you read a value from the instance. So for finalizable // objects that store a handle or pointer and provide a finalizer that // cleans them up, this can cause subtle race conditions with the finalizer // thread. This isn't just about handles - it can happen with just // about any finalizable resource. // // Users should insert a call to this method right after the last line // of their code where their code still needs the object to be kept alive. // The object which reference is passed into this method will not // be eligible for collection until the call to this method happens. // Once the call to this method has happened the object may immediately // become eligible for collection.

For example, consider the following user code, where a SKPaint is created without a using statement (so it isn't disposed explicitly):

void DrawSomething(SKCanvas canvas)
{
    var paint = new SKPaint
    {
        Style = SKPaintStyle.Stroke,
        StrokeWidth = 2f
    };

    canvas.DrawLine(0, 0, 50, 50, paint);
}

canvas.DrawLine(...) will retrieve the Handle of the SKPaint and SKCanvas, and pass it to the native sk_canvas_draw_line function:

https://github.com/mono/SkiaSharp/blob/a290ccffc74bc95a45569e8b74e77be851d4a688/binding/SkiaSharp/SKCanvas.cs#L89-L94

However, because the SKPaint object in the example above isn't used after the canvas.DrawLine() call, it may become eligible for garbage collection right after retrieving its Handle property in SKCanvas.DrawLine().

This could mean that the GC finalizer already calls SkiaApi.sk_compatpaint_delete (from the finalizer thread) while SkiaApi.sk_canvas_draw_line() is still executing, or even before SkiaApi.sk_canvas_draw_line is called, which would cause undefned behavior, like invalid memory access (e.g. it could cause Access Violations such as in #2794, though I don't know whether that issue would be caused by this one).

For example, this might be an issue when using RichString from Topten.RichTextKit, which doesn't seem to explicitly dispose SK objects like SKFont etc., and relies on the finalizer freeing the handle.

A solution is to add GC.KeepAlive() calls after a native API call (as described in that method's comment), to keep the objects alive from which the handles are retrieved. For example, with SKCanvas.DrawLine(), the code could be changed to this:

public void DrawLine (float x0, float y0, float x1, float y1, SKPaint paint)
{
    if (paint == null)
        throw new ArgumentNullException (nameof (paint));
    SkiaApi.sk_canvas_draw_line (Handle, x0, y0, x1, y1, paint.Handle);

    // Keep the objects with handles alive until after the native API call returns.
    GC.KeepAlive (this);
    GC.KeepAlive (paint);
}

The GC.KeepAlive() calls here ensure that the objects (SKPaint and SKCanvas) don't become eligible for GC (and thus their finalizer which would delete the native handles won't be called) until after the SkiaApi.sk_canvas_draw_line call returns.

This is also e.g. how dotnet/winforms handles it: https://github.com/dotnet/winforms/blob/897c9d87e973be3724375f8b272e82a5c6692070/src/System.Drawing.Common/src/System/Drawing/Bitmap.cs#L179-L180

What do you think? Thanks!

Code

(see above)

Expected Behavior

SKNativeObject instances should be kept alive during native API calls where their Handle is passed, until after the native API call returns.

Actual Behavior

SKNativeObject instances are not kept alive, which could cause the GC calling the finalizer which would delete the handle via functions such as SkiaApi.sk_compatpaint_delete while or before another thread still uses these handles in a native API call like SkiaApi.sk_canvas_draw_line, causing undefined behavior.

Version of SkiaSharp

3.x (Alpha)

Last Known Good Version of SkiaSharp

Other (Please indicate in the description)

IDE / Editor

Visual Studio (Windows)

Platform / Operating System

Windows

Platform / Operating System Version

Windows 10 Pro Version 22H2 (x64)

Devices

No response

Relevant Screenshots

No response

Relevant Log Output

No response

Code of Conduct