Skip to content

Fix IndexOutOfRangeException in FillRegionProcessor - #964

Merged
tocsoft merged 4 commits into
SixLabors:masterfrom
101100:fix-indexoutofrangeexception-in-fillregionprocessor
Aug 11, 2019
Merged

Fix IndexOutOfRangeException in FillRegionProcessor#964
tocsoft merged 4 commits into
SixLabors:masterfrom
101100:fix-indexoutofrangeexception-in-fillregionprocessor

Conversation

@101100

@101100 101100 commented Aug 10, 2019

Copy link
Copy Markdown
Contributor

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following matches the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

This is a fix for an issue that I have discovered as well as the reported issue #928. I have added two tests to illustrate the issue that fail on master as well as a possible fix.

The issue arises due to this code:

ImageSharp/src/ImageSharp.Drawing/Processing/Processors/Drawing/FillRegionProcessor{TPixel}.cs

Lines 99 to 112 in aac8eae

int pointsFound = region.Scan(subPixel + offset, buffer, configuration);
if (pointsFound == 0)
{
// nothing on this line, skip
continue;
}
QuickSort.Sort(buffer.Slice(0, pointsFound));
for (int point = 0; point < pointsFound; point += 2)
{
// points will be paired up
float scanStart = buffer[point] - minX;
float scanEnd = buffer[point + 1] - minX;

If pointsFound is odd (such as 3) and equal to the size of the buffer, the loop will go up to buffer.Length - 1, but that means that buffer[point + 1] will be out of bounds and cause an exception.

My solution was to add a second condition to the loop: point < buffer.Length - 1 Unfortunately, I don't know enough about the algorithm to know if this will result in an incorrect result.

@CLAassistant

CLAassistant commented Aug 10, 2019 •
edited
Loading

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@codecov

codecov Bot commented Aug 10, 2019

Copy link
Copy Markdown

Codecov Report

Merging #964 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #964      +/-   ##
==========================================
+ Coverage   89.65%   89.66%   +0.01%     
==========================================
  Files        1097     1097              
  Lines       48542    48573      +31     
  Branches     3425     3425              
==========================================
+ Hits        43520    43553      +33     
+ Misses       4319     4317       -2     
  Partials      703      703
Impacted Files Coverage Δ
.../Processors/Drawing/FillRegionProcessor{TPixel}.cs 97.61% <100%> (ø) ⬆️
...ageSharp.Tests/Drawing/FillRegionProcessorTests.cs 91.54% <100%> (+6.54%) ⬆️
...rawing/Processing/Extensions/DrawLineExtensions.cs 100% <0%> (+33.33%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aac8eae...f537d2e. Read the comment docs.

@codecov

codecov Bot commented Aug 10, 2019 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #964 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #964      +/-   ##
=========================================
+ Coverage   89.68%   89.7%   +0.01%     
=========================================
  Files        1097    1097              
  Lines       48474   48506      +32     
  Branches     3420    3420              
=========================================
+ Hits        43476   43510      +34     
+ Misses       4295    4293       -2     
  Partials      703     703
Impacted Files Coverage Δ
.../Processors/Drawing/FillRegionProcessor{TPixel}.cs 97.61% <100%> (ø) ⬆️
...ageSharp.Tests/Drawing/FillRegionProcessorTests.cs 91.66% <100%> (+6.66%) ⬆️
...rawing/Processing/Extensions/DrawLineExtensions.cs 100% <0%> (+33.33%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fb033fe...bff44f9. Read the comment docs.

@JimBobSquarePants

JimBobSquarePants commented Aug 11, 2019

Copy link
Copy Markdown
Member

Thanks! @101100 !!

We'll have a good look at that asap. 👍

tocsoft
tocsoft approved these changes Aug 11, 2019
View reviewed changes

@tocsoft tocsoft left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

looks good. Thanks for the fix

@tocsoft
tocsoft merged commit b238317 into SixLabors:master Aug 11, 2019
@101100 101100 mentioned this pull request Aug 11, 2019
Closed
@101100

101100 commented Aug 11, 2019

Copy link
Copy Markdown
Contributor Author

Thanks for the quick merge! I tested this in my project with the nightly build 1.0.0-dev002846 and it worked without an issue.

antonfirsov pushed a commit to antonfirsov/ImageSharp that referenced this pull request Nov 11, 2019
@tocsoft
Merge pull request SixLabors#964 from 101100/fix-indexoutofrangeexcep…
08644cf 
…tion-in-fillregionprocessor

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

Reviewers

@tocsoft tocsoft tocsoft approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@101100 @CLAassistant @JimBobSquarePants @tocsoft