fix x86 crash #5081
fix x86 crash #5081
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5081 +/- ##
=======================================
Coverage 75.66% 75.66%
=======================================
Files 993 993
Lines 178157 178157
Branches 19125 19125
=======================================
+ Hits 134800 134805 +5
+ Misses 38136 38134 -2
+ Partials 5221 5218 -3
|
| @@ -65,9 +65,9 @@ IPredictor IModelCombiner.CombineModels(IEnumerable<IPredictor> models) | |||
| foreach (var t in tree.TrainedEnsemble.Trees) | |||
| { | |||
| var bytes = new byte[t.SizeInBytes()]; | |||
| int position = -1; | |||
There was a problem hiding this comment.
Interesting. Can you please explain this issue a bit more? Why does this happen in x64 but not in x86? This is managed memory. Why is it corrupting the unamanaged heap?
There was a problem hiding this comment.
Yes, this is index out of range issue that corrupts memory but this one is index was out of range in former (start to use byte array from index -1)...
This memory corrupted (byte array) is allocated in managed but used as unmanaged (from fixed section in C# and pointer) like below:
https://github.com/dotnet/machinelearning/blob/master/src/Microsoft.ML.FastTree/TreeEnsemble/InternalRegressionTree.cs#L101
https://github.com/dotnet/machinelearning/blob/master/src/Microsoft.ML.FastTree/Utils/ToByteArrayExtensions.cs#L113
when position is -1, the pointer ((int*)(pBuffer + position)) is accessing memory it should not.
I'm still not sure why this issue not repro in x64. In theory this can also corrupt x64 memory.
In reply to: 419011127 [](ancestors = 419011127)
There was a problem hiding this comment.
SZArrays on x64 have a 4-byte padding between the length and the start of the data.
There was a problem hiding this comment.
I suspected this was the issue from reading above, but special thanks to @stephentoub for finding the actual code that proves it 👍
There was a problem hiding this comment.
@eerhardt also confirmed this in email.
@sharwell, @stephentoub and @eerhardt Thanks for shedding more light on the issue.
ghost
locked as resolved and limited conversation to collaborators
fixes #1216.
TreeEnsembleCombiner has a bug that causing byte array out of range and corrupts heap