Bug 59547 - Premature collection possible in bindings in MetalPerformanceShaders
Summary: Premature collection possible in bindings in MetalPerformanceShaders
Status: RESOLVED FIXED
Alias: None
Product: iOS
Classification: Xamarin
Component: Xamarin.iOS.dll (show other bugs)
Version: master
Hardware: PC Mac OS
: --- major
Target Milestone: xcode9.2
Assignee: Alex Soto [MSFT]
URL:
Depends on:
Blocks:
 
Reported: 2017-09-18 07:55 UTC by Rolf Bjarne Kvinge [MSFT]
Modified: 2017-11-28 23:10 UTC (History)
2 users (show)

Tags:
Is this bug a regression?: ---
Last known good build:

Notice (2018-05-24): bugzilla.xamarin.com is now in read-only mode.

Please join us on Visual Studio Developer Community and in the Xamarin and Mono organizations on GitHub to continue tracking issues. Bugzilla will remain available for reference in read-only mode. We will continue to work on open Bugzilla bugs, copy them to the new locations as needed for follow-up, and add the new items under Related Links.

Our sincere thanks to everyone who has contributed on this bug tracker over the years. Thanks also for your understanding as we make these adjustments and improvements for the future.


Please create a new report on Developer Community or GitHub with your current version information, steps to reproduce, and relevant error messages or log files if you are hitting an issue that looks similar to this resolved bug and you do not yet see a matching new report.

Related Links:
Status:
RESOLVED FIXED

Description Rolf Bjarne Kvinge [MSFT] 2017-09-18 07:55:04 UTC
See: https://github.com/xamarin/xamarin-macios/blob/1690ccbc991ab59835f76a15ef510cd182259915/src/MetalPerformanceShaders/MPSKernel.cs#L23-L43

The following code is unsafe, because the GC is free to move the 'values' array once we're out of the fixed statement:

    fixed (nfloat *ptr = values)
        return (IntPtr) ptr;

this means that the calling function now has pointer to memory that the GC can move at any time.
Comment 1 Rolf Bjarne Kvinge [MSFT] 2017-09-18 07:57:18 UTC
Raising priority and targeting the next release (15.6), since this will cause random crashes (or other random behavior) when it occurs, and it will be very tricky to track down.
Comment 2 Sebastien Pouliot 2017-11-22 22:46:49 UTC
The manual bindings for CIVector has the same code.
Comment 3 Sebastien Pouliot 2017-11-24 15:05:33 UTC
CIVector fixed in https://github.com/xamarin/xamarin-macios/pull/3038

@Alex since you're working on MPS can you apply the same pattern on MPSKernel ? thanks!
Comment 4 Alex Soto [MSFT] 2017-11-26 21:18:13 UTC
PR: https://github.com/xamarin/xamarin-macios/pull/3005
Comment 5 Sebastien Pouliot 2017-11-27 22:05:39 UTC
Switching milestone to `xcode9.2` since it enables the framework on macOS (and will likely be merged into 15.5)
Comment 6 Alex Soto [MSFT] 2017-11-28 23:10:27 UTC
Fixed in xamarin-macios/xcode9.2 @ https://github.com/xamarin/xamarin-macios/commit/87f9e239898c16a64de153e6fb631b7ad8fb3865