Skip to content

fix x86 crash #5081

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.

Sign up for GitHub

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

Merged
frank-dong-ms-zz merged 1 commit into from
May 4, 2020
Merged

fix x86 crash #5081

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/Microsoft.ML.FastTree/TreeEnsemble/TreeEnsembleCombiner.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

@harishsk harishsk May 2, 2020

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@frank-dong-ms-zz frank-dong-ms-zz May 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

@sharwell sharwell May 4, 2020

Choose a reason for hiding this comment

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

SZArrays on x64 have a 4-byte padding between the length and the start of the data.

https://github.com/dotnet/runtime/blob/200b197559fc49f91b8db3f20fd6df6b5660d7bf/src/coreclr/src/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.CoreCLR.cs#L306-L321

Copy link
Contributor

@sharwell sharwell May 4, 2020

Choose a reason for hiding this comment

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

I suspected this was the issue from reading above, but special thanks to @stephentoub for finding the actual code that proves it 👍

Copy link
Contributor

@harishsk harishsk May 4, 2020

Choose a reason for hiding this comment

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

@eerhardt also confirmed this in email.
@sharwell, @stephentoub and @eerhardt Thanks for shedding more light on the issue.

int position = 0;
t.ToByteArray(bytes, ref position);
position = -1;
position = 0;
var tNew = new InternalRegressionTree(bytes, ref position);
if (paramA != 1)
{
Expand Down
8 changes: 4 additions & 4 deletions test/Microsoft.ML.Predictor.Tests/TestPredictors.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -625,7 +625,7 @@ public void RankingLightGBMTest()
Done();
}

[X64Fact("x86 fails. Associated GitHubIssue: https://github.com/dotnet/machinelearning/issues/1216")]
[Fact]
public void TestTreeEnsembleCombiner()
{
var dataPath = GetDataPath("breast-cancer.txt");
Expand All @@ -646,7 +646,7 @@ public void TestTreeEnsembleCombiner()
CombineAndTestTreeEnsembles(dataView, fastTrees);
}

[X64Fact("x86 fails. Associated GitHubIssue: https://github.com/dotnet/machinelearning/issues/1216")]
[Fact]
public void TestTreeEnsembleCombinerWithCategoricalSplits()
{
var dataPath = GetDataPath("adult.tiny.with-schema.txt");
Expand Down Expand Up @@ -755,7 +755,7 @@ private void CombineAndTestTreeEnsembles(IDataView idv, PredictorModel[] fastTre
}
}

[X64Fact("x86 fails. Associated GitHubIssue: https://github.com/dotnet/machinelearning/issues/1216")]
[Fact]
public void TestEnsembleCombiner()
{
var dataPath = GetDataPath("breast-cancer.txt");
Expand Down Expand Up @@ -801,7 +801,7 @@ public void TestEnsembleCombiner()
Done();
}

[X64Fact("x86 fails. Associated GitHubIssue: https://github.com/dotnet/machinelearning/issues/1216")]
[LightGBMFact]
public void TestMulticlassEnsembleCombiner()
{
var dataPath = GetDataPath("breast-cancer.txt");
Expand Down