Skip to content

Conversation

@cshung
Copy link
Contributor

@cshung cshung commented Oct 9, 2021

Backporting #60189

Customer Impact

Since the GetObjectGeneration() API returned -1 instead of 2 for a Gen 2 object lying on the ephemeral heap segment. A less defensive ICorProfiler implementation might index into the -1 entry of an array and leading to buffer underflow. Even for a defensive implementation, we would have wrong statistics.

After the change, we will output 2 as the correct generation number.

Testing

The diagnostics team confirmed the change fixed the problem through their automated testing.

Risk

Low – this is a new code path introduced in .NET 6.

@ghost ghost added the area-GC-coreclr label Oct 9, 2021
@ghost
Copy link

ghost commented Oct 9, 2021

Tagging subscribers to this area: @dotnet/gc
See info in area-owners.md if you want to be subscribed.

Issue Details

WIP

Author: cshung
Assignees: -
Labels:

area-GC-coreclr

Milestone: -

@cshung cshung requested a review from Maoni0 October 9, 2021 16:49
@Maoni0
Copy link
Member

Maoni0 commented Oct 9, 2021

just a comment on your description of the bug -

Since the GetObjectGeneration() API returned -1 instead of 2 for a Gen 2 object lying on the ephemeral heap segment. A careless ICorProfiler implementation might index into the -1 entry

I'm confused why you would categorize this ICorProfiler implementation as "careless", is -1 considered a valid value that means gen2 for profilers?

@cshung
Copy link
Contributor Author

cshung commented Oct 9, 2021

I'm confused why you would categorize this ICorProfiler implementation as "careless", is -1 considered a valid value that means gen2 for profilers?

No. -1 is not considered as a valid value to mean gen2. Perhaps "less defensive" is a better description than "careless" in this case. One could imagine a less defensive implementation look like this.

int gen = GetObjectGeneration(...);
counter[gen]++;

or a more defensive one look like this:

int gen = GetObjectGeneration(...);
if (0 <= gen && gen <= 4)
{
  counter[gen]++;
}
else
{
  assert(false);
}

In the former case, we will have a buffer underflow issue, in the latter case, the statistics are wrong. Either case is not great, but the former one is worse.

@Anipik
Copy link
Contributor

Anipik commented Oct 11, 2021

approved in email

@Anipik Anipik merged commit b1aedd5 into dotnet:release/6.0 Oct 11, 2021
@Anipik Anipik added the Servicing-approved Approved for servicing release label Oct 11, 2021
@cshung cshung deleted the public/backport-fix-get-generation-with-range branch October 11, 2021 16:05
@ghost ghost locked as resolved and limited conversation to collaborators Nov 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-GC-coreclr Servicing-approved Approved for servicing release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants