Skip to content

Conversation

@antonfirsov
Copy link
Member

@antonfirsov antonfirsov commented Oct 1, 2021

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 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 fixes an overlook in #1398 (which I was reviewing ...), that exposes a potential memory corruption.

source can point to a managed array here, meaning if we don't ask for pinning explicitly, the GC can move the backing array, invalidating the destination address pointed by sourceBase.

In practice this almost never happens since big image arrays are allocated on the LOH today, however I still can imagine an unlucky situation with small images where this causes an error.

AsPointer should be used with care, only in cases when we know the destination reference is part of a pinned, native or stack object.

@codecov
Copy link

codecov bot commented Oct 1, 2021

Codecov Report

Merging #1770 (6f5269f) into master (4ce2d0c) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1770      +/-   ##
==========================================
- Coverage   84.49%   84.49%   -0.01%     
==========================================
  Files         849      849              
  Lines       37413    37412       -1     
  Branches     4376     4376              
==========================================
- Hits        31612    31611       -1     
  Misses       4947     4947              
  Partials      854      854              
Flag Coverage Δ
unittests 84.49% <100.00%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
...mageSharp/Common/Helpers/SimdUtils.HwIntrinsics.cs 99.13% <100.00%> (-0.01%) ⬇️

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 4ce2d0c...6f5269f. Read the comment docs.

Copy link
Member

@JimBobSquarePants JimBobSquarePants left a comment

Choose a reason for hiding this comment

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

Good catch!

@br3aker
Copy link
Contributor

br3aker commented Oct 2, 2021

Isn't it better to use ref's and Unsafe.Add instead of pointer arithmetic? This would eliminate the need to pin input span. But I'm not so sure about performance in comparison to raw pointers here.

@antonfirsov
Copy link
Member Author

antonfirsov commented Oct 2, 2021

Isn't it better to use ref's and Unsafe.Add instead of pointer arithmetic?

Yes, JIT should generate the same machine code for ref -> value loading and it's generally better to avoid pinning, but I wanted a very quick 5 minute fix for now to make sure there are no security bugs in the repo. Did a quick run of our Rgba32 -> Vector4 benchmarks, and nothing changed, so decided to not bother anymore. May push a better fix later when I have more time dedicated to code around SIMD stuff.

@JimBobSquarePants
Copy link
Member

We can deal with that kind of improvement for V3. I’d like to focus now on simply shipping something so we can finally ditch the legacy and build what we want to.

@br3aker
Copy link
Contributor

br3aker commented Oct 2, 2021

it's generally better to avoid pinning

As you've already said, this code usually works with LOH memory so pinning should be almost free. My main point was about more 'C#'ish code :)
But you're right, it's better to fix problems within a quick timespan than spend weeks writing 'best of best' code.

@antonfirsov
Copy link
Member Author

@JimBobSquarePants codecov here fails with a -0.01% change, not sure what is the reason. Can't we at least set a threshold different than 0.0 %?

@JimBobSquarePants
Copy link
Member

Can't we at least set a threshold different than 0.0 %?

The only thing I can see that might allow us to toggle this is the precision field. We could try updating that in the repo codecov.yml file and see what happens?
https://docs.codecov.com/docs/codecovyml-reference#coverageprecision

@JimBobSquarePants JimBobSquarePants merged commit 2410a37 into master Oct 5, 2021
@JimBobSquarePants JimBobSquarePants deleted the af/pin-ByteToNormalizedFloat branch October 5, 2021 01:26
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.

4 participants