microsoft / clrmd

Microsoft.Diagnostics.Runtime is a set of APIs for introspecting processes and dumps.
MIT License
1.05k stars 254 forks source link

Valid/null inconsistency in docs #871

Open erikness opened 3 years ago

erikness commented 3 years ago

In ClrObject:

        /// <summary>
        /// Returns whether this is a valid object.  This will return null
        /// </summary>
        public bool IsValid => Address != 0 && Type != null;

        /// <summary>
        /// Returns if the object value is <see langword="null"/>.
        /// </summary>
        public bool IsNull => Address == 0;

(Aside: "This will return null" seems like it was a sentence that wasn't finished.)

vs GettingStarted.md:

  1. IsValid tells you if the object is valid. This will return false if the address in the target address space did not point to a valid object.
  2. IsNull tells you if the object address was null (also Address == 0 in this case). Note that null objects are considered valid (IsNull == true => IsValid == true).

Are null objects valid? The docs say yes, but the code says no. If an object is null, its Address is 0, which would short-circuit the condition for IsValid, making it false.

erikness commented 3 years ago

The code also implies that if I check IsValid when enumerating objects in the heap:

if (!heapObject.IsValid)
{
    continue;
}

I don't have to check IsNull.

leculver commented 3 years ago

That documentation definitely needs to be cleaned up. Ignoring the documentation issue, the code as written is what the behavior should be. If ClrObject.Address == 0 then both IsNull == true and IsValid == false. Essentially, IsValid is meant to say "Does this ClrObject point to an actual, valid object?" and null is not a valid object. So the logic above is correct.

Though of course, we could also clean up IsValid to make it simply: public bool IsValid =>Type != null; because if Address is 0 the object can't have a valid type, so the Address != 0 check should not needed...but I tend to program things defensively and so the extra check will stay there.

erikness commented 3 years ago

I agree with that. One thing you might add to the docs as well is a short description of what kind of objects could be invalid, aside from null ones. That is: in what cases would a ClrObject have Type == null but Address != null? (IsValid in ClrValueType hints at issues with metadata or generics).

leculver commented 3 years ago

That's only for a ClrValueType. The only way to get a null ClrObject.Type is if a value that's supposed to point to an object instead points to bogus memory, or if the ClrObject points to what should be a valid object but the MethodTable of that object has been overwritten. Both cases are usually caused by memory corruption.

There's a rare case where sometimes the GC could be in the middle of updating data structures and a crash dump was taken during a tiny window when the heap isn't walkable. That could also lead to ClrHeap.EnumerateObjects trying to point to a ClrObject which has an "invalid" methodtable which would have been valid if the dump had been taken a few milliseconds later. This is very rare.

In short, .IsValid should only return false if the object is null, there's heap corruption, the crash dump was taken in an "unlucky" window, or a bug in ClrMD which we should fix.