Skip to content

Conversation

@merriemcgaw
Copy link
Member

@merriemcgaw merriemcgaw commented Dec 5, 2025

Fixes #14113

Description

This PR addresses a crash (AccessViolationException in .NET 8, ExecutionEngineException in .NET 9+) that occurs when calling \Graphics.FillRectangle\ with a specific combination of parameters:

  • \PixelFormat.Format24bppRgb\
  • \SmoothingMode.AntiAlias\
  • Drawing outside the bitmap bounds

This is a known GDI+ bug where it writes out of bounds. In .NET 9, the runtime treats this memory corruption as fatal and terminates the process.

Fix

Added parameter validation in \Graphics.FillRectangle\ to detect this specific unsafe combination and throw a managed \ArgumentException\ instead of allowing the process to crash.

Regression

This is a fix for a crash that became fatal in .NET 9 due to stricter runtime exception handling.

Testing

Verified with a reproduction app that previously crashed the process and now correctly throws an ArgumentException.

Microsoft Reviewers: Open in CodeFlow

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a critical crash that occurs when calling Graphics.FillRectangle with a specific combination of parameters: PixelFormat.Format24bppRgb, SmoothingMode.AntiAlias, and drawing coordinates outside the bitmap bounds. This GDI+ bug causes an AccessViolationException in .NET 8 and an ExecutionEngineException in .NET 9+, with the latter being fatal and terminating the process.

Key Changes:

  • Added parameter validation in Graphics.FillRectangle to detect the unsafe combination and throw a managed ArgumentException before GDI+ can crash
  • Added a test case to verify the fix throws the expected exception instead of crashing

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.

File Description
src/System.Drawing.Common/src/System/Drawing/Graphics.cs Added bounds validation check in FillRectangle method to prevent GDI+ crash when using AntiAlias with 24bppRgb format
src/System.Drawing.Common/tests/System/Drawing/GraphicsTests.cs Added test to verify ArgumentException is thrown for the problematic combination instead of crashing

@codecov
Copy link

codecov bot commented Dec 5, 2025

Codecov Report

❌ Patch coverage is 81.20567% with 53 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.15548%. Comparing base (ae5cd21) to head (ff893a1).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@                 Coverage Diff                 @@
##                main      #14115         +/-   ##
===================================================
- Coverage   77.15789%   77.15548%   -0.00241%     
===================================================
  Files           3279        3279                 
  Lines         645317      645599        +282     
  Branches       47718       47761         +43     
===================================================
+ Hits          497913      498115        +202     
- Misses        143717      143783         +66     
- Partials        3687        3701         +14     
Flag Coverage Δ
Debug 77.15548% <81.20567%> (-0.00241%) ⬇️
integration 18.97260% <1.35747%> (-0.01322%) ⬇️
production 52.03427% <76.01810%> (+0.00903%) ⬆️
test 97.40793% <100.00000%> (+0.00043%) ⬆️
unit 49.48328% <76.01810%> (+0.00681%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@KlausLoeffelmann KlausLoeffelmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!
:shipit:

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.

Comment on lines +3068 to +3076
RectangleF[] rects = new[]
{
new RectangleF(190.5f, 180.5f, 100, 100), // Issue repro
new RectangleF(-100, 50, 50, 50), // Fully out left
new RectangleF(300, 50, 50, 50), // Fully out right
new RectangleF(-10, -10, 50, 50), // Partial top-left
new RectangleF(200, 200, 100, 100), // Partial bottom-right
new RectangleF(50, 50, 50, 50) // Fully inside
};
Copy link

Copilot AI Dec 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the coding guidelines (section 1.2), prefer modern collection initializers using [] syntax:

RectangleF[] rects =
[
    new RectangleF(190.5f, 180.5f, 100, 100), // Issue repro
    new RectangleF(-100, 50, 50, 50),         // Fully out left
    new RectangleF(300, 50, 50, 50),          // Fully out right
    new RectangleF(-10, -10, 50, 50),         // Partial top-left
    new RectangleF(200, 200, 100, 100),       // Partial bottom-right
    new RectangleF(50, 50, 50, 50)            // Fully inside
];

Copilot generated this review using guidance from repository custom instructions.
Comment on lines +3062 to +3066
public void FillRectangles_AntiAlias_24bppRgb_OutOfBounds_DoesNotThrow()
{
using Bitmap bmp = new(256, 256, PixelFormat.Format24bppRgb);
using Graphics g = Graphics.FromImage(bmp);
g.SmoothingMode = SmoothingMode.AntiAlias;
Copy link

Copilot AI Dec 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] For consistency with the other test methods in this file (e.g., lines 3017, 3033, 3043, 3054), consider using the variable name graphics instead of g for the Graphics instance.

Copilot uses AI. Check for mistakes.
Comment on lines +3082 to +3086
public void DrawImage_Float_AntiAlias_24bppRgb_OutOfBounds_DoesNotThrow()
{
using Bitmap bmp = new(256, 256, PixelFormat.Format24bppRgb);
using Graphics g = Graphics.FromImage(bmp);
g.SmoothingMode = SmoothingMode.AntiAlias;
Copy link

Copilot AI Dec 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] For consistency with the other test methods in this file (e.g., lines 3017, 3033, 3043, 3054), consider using the variable name graphics instead of g for the Graphics instance.

Copilot uses AI. Check for mistakes.
Comment on lines +3103 to +3107
public void DrawImage_RectF_AntiAlias_24bppRgb_OutOfBounds_DoesNotThrow()
{
using Bitmap bmp = new(256, 256, PixelFormat.Format24bppRgb);
using Graphics g = Graphics.FromImage(bmp);
g.SmoothingMode = SmoothingMode.AntiAlias;
Copy link

Copilot AI Dec 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] For consistency with the other test methods in this file (e.g., lines 3017, 3033, 3043, 3054), consider using the variable name graphics instead of g for the Graphics instance.

Copilot uses AI. Check for mistakes.
Comment on lines +1225 to +1278
// GDI+ has a bug where it writes out of bounds when using AntiAlias smoothing mode
// on a 24bppRgb bitmap, causing an AccessViolationException (or ExecutionEngineException in .NET 9+).
// We validate the parameters here to prevent the crash.
if (SmoothingMode == Drawing2D.SmoothingMode.AntiAlias &&
_backingImage is { PixelFormat: PixelFormat.Format24bppRgb })
{
float left = x;
float right = x + width;
float top = y;
float bottom = y + height;

if (width < 0)
{
left = right;
right = x;
}

if (height < 0)
{
top = bottom;
bottom = y;
}

if (left < 0)
{
left = 0;
}

if (right > _backingImage.Width)
{
right = _backingImage.Width;
}

if (top < 0)
{
top = 0;
}

if (bottom > _backingImage.Height)
{
bottom = _backingImage.Height;
}

if (left >= right || top >= bottom)
{
// The rectangle is completely outside the image bounds.
return;
}

x = left;
y = top;
width = right - left;
height = bottom - top;
}
Copy link

Copilot AI Dec 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This clipping logic is duplicated across four methods (FillRectangle, FillRectangles, and two DrawImage overloads). Consider extracting this into a private helper method to improve maintainability. For example:

private bool ShouldClipForAntiAlias24bppRgb(out int imageWidth, out int imageHeight)
{
    imageWidth = 0;
    imageHeight = 0;
    
    if (SmoothingMode != Drawing2D.SmoothingMode.AntiAlias ||
        _backingImage is not { PixelFormat: PixelFormat.Format24bppRgb })
    {
        return false;
    }
    
    imageWidth = _backingImage.Width;
    imageHeight = _backingImage.Height;
    return true;
}

private static void NormalizeRectangle(float x, float y, float width, float height, 
    out float left, out float right, out float top, out float bottom)
{
    left = x;
    right = x + width;
    top = y;
    bottom = y + height;
    
    if (width < 0)
    {
        left = right;
        right = x;
    }
    
    if (height < 0)
    {
        top = bottom;
        bottom = y;
    }
}

private static void ClipRectangle(ref float left, ref float right, ref float top, ref float bottom, 
    int maxWidth, int maxHeight)
{
    if (left < 0) left = 0;
    if (right > maxWidth) right = maxWidth;
    if (top < 0) top = 0;
    if (bottom > maxHeight) bottom = maxHeight;
}

This would reduce duplication and make the code more maintainable.

Copilot uses AI. Check for mistakes.
public void DrawImage(Image image, RectangleF destRect, RectangleF srcRect, GraphicsUnit srcUnit)
{
ArgumentNullException.ThrowIfNull(image);

Copy link

Copilot AI Dec 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the coding guidelines (section 1.11), an empty line should be inserted after structure blocks. Add an empty line between ArgumentNullException.ThrowIfNull(image); and the start of the clipping logic to improve readability.

Copilot generated this review using guidance from repository custom instructions.
float destW = right - left;
float destH = bottom - top;

if (Math.Abs(destW) > float.Epsilon && Math.Abs(destH) > float.Epsilon)
Copy link

Copilot AI Dec 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check Math.Abs(destW) > float.Epsilon may not be the most appropriate for graphics dimensions. float.Epsilon is the smallest positive value greater than zero (approximately 1.4e-45), which is extremely small. For graphics operations, consider using a more reasonable threshold for zero comparisons, such as 0.001f or simply checking for exact zero if negative dimensions are normalized earlier. The current check would allow extremely small non-zero values that could still cause issues in division operations.

Copilot uses AI. Check for mistakes.
float destW = right - left;
float destH = bottom - top;

if (Math.Abs(destW) > float.Epsilon && Math.Abs(destH) > float.Epsilon)
Copy link

Copilot AI Dec 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check Math.Abs(destW) > float.Epsilon may not be the most appropriate for graphics dimensions. float.Epsilon is the smallest positive value greater than zero (approximately 1.4e-45), which is extremely small. For graphics operations, consider using a more reasonable threshold for zero comparisons, such as 0.001f or simply checking for exact zero if negative dimensions are normalized earlier. The current check would allow extremely small non-zero values that could still cause issues in division operations.

Copilot uses AI. Check for mistakes.
public void DrawImage(Image image, float x, float y, float width, float height)
{
ArgumentNullException.ThrowIfNull(image);

Copy link

Copilot AI Dec 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the coding guidelines (section 1.11), an empty line should be inserted after structure blocks. Add an empty line between ArgumentNullException.ThrowIfNull(image); and the start of the clipping logic to improve readability.

Copilot generated this review using guidance from repository custom instructions.
@KlausLoeffelmann
Copy link
Member

@copilot: I am wondering. If we defined a clipping region temporarily, would that have a chance to mitigate the underlying access violation?

Copy link
Contributor

Copilot AI commented Dec 6, 2025

@KlausLoeffelmann I've opened a new pull request, #14117, to work on those changes. Once the pull request is ready, I'll request review from you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Crash (AccessViolationException/ExecutionEngineException) graphics.FillRectangle into Bitmap with Format24bppRgb

3 participants