Skip to content
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.

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

Fix issue 47805 #52183

Merged
merged 38 commits into from
May 4, 2021
Merged

Fix issue 47805 #52183

merged 38 commits into from
May 4, 2021

Conversation

PeterSolMS
Copy link
Contributor

Fix issue 47805 - in this case, the BGC_MARKED_BY_FGC bit (0x2) set in the method table leaked out and caused issues for the user program.

In the cases that I've been able to repro, this happened because the bit got set for a short object right in front of a pinned plug, and then saved away by enque_pinned_plug.

Later on, in the case of mark & sweep, we check for the bit and reset it, but later we copy the saved object back by calling recover_saved_pinned_info which calls recover_plug_info to do the actual work. This isn't a problem for the compact case, because we copy the saved object back during compact_plug, turn the bit off, then save it again at the end of compact_plug.

The fix is to turn off the extra bits at the beginning of enque_pinned_plug and save_post_plug_info for the copy that is later restored in mark & sweep (there are actually two copies saved, one for use during compact and one for use during mark & sweep). This builds on an earlier fix by Maoni for a similar problem with another bit.

…n the method table leaked out and caused issues for the user program.

In the cases that I've been able to repro, this happened because the bit got set for a short object right in front of a pinned plug, and then saved away by enque_pinned_plug.

Later on, in the case of mark & sweep, we check for the bit and reset it, but later we copy the saved object back by calling recover_saved_pinned_info which calls recover_plug_info to do the actual work. This isn't a problem for the compact case, because we copy the saved object back during compact_plug, turn the bit off, then save it again at the end of compact_plug.

The fix is to turn off the extra bits at the beginning of enque_pinned_plug and save_post_plug_info for the copy that is later restored in mark & sweep (there are actually two copies saved, one for use during compact and one for use during mark & sweep). This builds on an earlier fix by Maoni for a similar problem with another bit.
@ghost
Copy link

ghost commented May 3, 2021

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

Issue Details

Fix issue 47805 - in this case, the BGC_MARKED_BY_FGC bit (0x2) set in the method table leaked out and caused issues for the user program.

In the cases that I've been able to repro, this happened because the bit got set for a short object right in front of a pinned plug, and then saved away by enque_pinned_plug.

Later on, in the case of mark & sweep, we check for the bit and reset it, but later we copy the saved object back by calling recover_saved_pinned_info which calls recover_plug_info to do the actual work. This isn't a problem for the compact case, because we copy the saved object back during compact_plug, turn the bit off, then save it again at the end of compact_plug.

The fix is to turn off the extra bits at the beginning of enque_pinned_plug and save_post_plug_info for the copy that is later restored in mark & sweep (there are actually two copies saved, one for use during compact and one for use during mark & sweep). This builds on an earlier fix by Maoni for a similar problem with another bit.

Author: PeterSolMS
Assignees: -
Labels:

area-GC-coreclr

Milestone: -

Comment on lines 20929 to 20945
#if defined(SHORT_PLUGS) || defined(DOUBLY_LINKED_FL)
// In the case of short plugs or doubly linked free lists, there may be extra bits
// set in the method table pointer.
// Clear these bits for the copy saved in saved_pre_plug, but not for the copy
// saved in saved_pre_plug_reloc.
// This is because we need these bits for compaction, but not for mark & sweep.
MethodTable* pRawMethodTable = header(last_object_in_last_plug)->RawGetMethodTable();
MethodTable* pMethodTable = header(last_object_in_last_plug)->GetMethodTable();
if (pRawMethodTable != pMethodTable)
{
header(last_object_in_last_plug)->RawSetMethodTable (pMethodTable);
dprintf (3, ("method table for plug %Ix has low bits set: %Ix",
last_object_in_last_plug,
((size_t)pRawMethodTable) - ((size_t)pMethodTable)));
}
#endif //SHORT_PLUGS || DOUBLY_LINKED_FL
// now copy the bits over
Copy link
Member

Choose a reason for hiding this comment

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

it would be a bit nicer to follow the current coding convention, perhaps something like?

inline
size_t clear_special_bits_mt (uint8_t* obj)
{
#if defined(SHORT_PLUGS) || defined(DOUBLY_LINKED_FL)
    size_t special_bits = header(node)->ClearSpecialBits();
    // could also assert to make sure no special bits are set if neither define is defined
#endif //SHORT_PLUGS || DOUBLY_LINKED_FL
}

inline
void set_special_bits_mt (uint8_t* obj, size_t special_bits)
{
#if defined(SHORT_PLUGS) || defined(DOUBLY_LINKED_FL)
    header(node)->SetSpecialBits (special_bits);
#endif //SHORT_PLUGS || DOUBLY_LINKED_FL
}

size_t special_bits = clear_special_bits_mt (last_object_in_last_plug);
memcpy (&(m.saved_post_plug), m.saved_post_plug_info_start, sizeof (gap_reloc_pair));
set_special_bits_mt (last_object_in_last_plug, special_bits);

Copy link
Member

@Maoni0 Maoni0 left a comment

Choose a reason for hiding this comment

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

other than the coding convention part, it LGTM!

@PeterSolMS PeterSolMS merged commit b7457e8 into dotnet:main May 4, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants