Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix IndexOutOfRangeException in FillRegionProcessor #964

Merged
merged 4 commits into from Aug 11, 2019
Merged

Fix IndexOutOfRangeException in FillRegionProcessor #964

merged 4 commits into from Aug 11, 2019

Conversation

101100
Copy link
Contributor

@101100 101100 commented Aug 10, 2019

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:

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
Copy link

CLAassistant commented Aug 10, 2019

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Aug 10, 2019

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
Copy link

codecov bot commented Aug 10, 2019

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
Copy link
Member

Thanks! @101100 !!

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

Copy link
Member

@tocsoft tocsoft 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. Thanks for the fix

@101100
Copy link
Contributor Author

101100 commented Aug 11, 2019

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
…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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants