Cook non-determinism when serializing UObject pointers pointing to garbage

Development Pipeline & Plugins
, ,
1

There’s a cook determinism issue in the way UObject unversioned serialization handles UObject pointers pointing to garbage. Depending on GC timing, it can either be skipped (as a property matching CDO) or serialized explicitly. In more depth:

  • The scenario I’ve noticed it in was a blueprint that had a construction script that destroys a component.
  • When the level is loaded, the property containing component pointer is deserialized as null.
  • When later the construction scripts are executed, first simple construction script creates the component (and assigns pointer to it to the property), then user construction script deletes it (and marks component as garbage, *not* touching the property - so property now points to a UObject pending GC).

Now, one of the two things might happen.

  • If GC happens before save:
    • GC destroys the component and zeros out all references (the actor’s property, in this case)
    • Then during serialization the unversioned serialization code finds that the property value (zero) matches CDO (zero) and marks it as ‘skipped’ in SerializeUnversionedProperties
  • If GC does not happen before save:
    • During dependency gathering, property is serialized, then FPackageHarvester calls FSaveContext::IsUnsaveable and finds that property is garbage and thus is to be skipped
    • During actual serialization, SerializeUnversionedProperties sees that property is not equal to CDO (non-zero vs zero) and marks it as ‘serialized’; it’s also not zero, so it doesn’t use the zero-mask serialization path and does a proper serialization.
    • The serialization function then tries to lookup FPackageIndex corresponding to the object, doesn’t find it (because FPackageHarvester skipped it) and writes out invalid-index for the data

So ultimately the serialized state is equivalent, but has different representation (`property has default value` vs `property has non-default value, is non-zero, and it’s value is zero`).

2

Steps to Reproduce

  1. Create an actor blueprint and add any component
  2. Add a ‘Destroy component’ node into construction script to immediately destroy the component
  3. Place an actor blueprint on a level
  4. Put a breakpoint in AActor::Serialize and cook the level.
  5. On first hit for the actor we’ve placed (i.e. load), find the property pointer (since it’s a dynamic property, you need to explore UClass properties, find the offset, then do the manual pointer math), and put a write breakpoint on the pointer and on the OwnedComponents.Num
  6. Observe that component is created, added to OwnedComponents and to the property pointer while running simple construction script
  7. Observe that component is removed from OwnedComponents and then MarkAsGarbage’d while running user construction script
  8. Observe that during serialization, the property value is a UObject ptr with Garbage flag set.
  9. Observe that hw breakpoint on property is hit during next GC when the property is zerod out.

8 and 9 can happen in reverse order.

3

FWIW a workaround I’m using now:

First, in FObjectProperty::Identical and FObjectPropertyBase::Identical add the following:

if (ObjectA && !IsValidChecked(ObjectA))
		ObjectA = nullptr;
	if (ObjectB && !IsValidChecked(ObjectB))
		ObjectB = nullptr;

This ensures that UObject pointers pointing to garbage are considered equivalent to null when doing Identical check.

Then, add a new virtual function FProperty::IsLogicalZeroForSerialization, which by default returns false, and for FObjectProperty[Base] is implemented as

bool FObjectPropertyBase::IsLogicalZeroForSerialization(const void* V) const
{
	return !IsValid(V ? GetObjectPropertyValue(V) : nullptr);
}

Finally, call it in FUnversionedPropertySerializer::ShouldSaveAsZero:

if (!FastZeroIntNum)
		{
			return false;
		}
		auto V = GetValue(Data);
 
		if (bIsOptional)
		{
			return !static_cast<const FOptionalProperty*>(Property)->IsSet(V);
		}
		else if (Property->IsLogicalZeroForSerialization(V))
		{
			return true;
		}
		else if (FastZeroIntNum == 1) // Can likely be simplified and faster using a switch() statement like LoadZero()
		{
			return IsIntZero(V, IntType);
		}
		else
		{
			return IsIntRangeZero(V, FastZeroIntNum, IntType);
		}

This ensures that pointer-to-garbage, if CDO has non-zero value, uses special zero serialization path (ie zero mask bit)

4

Thanks for the in-depth analysis, that analysis reduces the time for us to submit a fix for the issue by 90%.

Your workaround is the direction I was thinking of going when I read the analysis, but I want to see if there’s a good way to make its impact on the engine’s behavior narrower, so that it is only used during SavePackage. I’ll discuss with other programmers here and let you know when we submit a solution, either yours or something similar.

5

Hi Andrew,

As Matt said, thanks for the detailed info and fix suggestion, we’ll get that fixed for 5.7 and we’ll also report back here when we have something lined up and submitted. It might be slightly different than your current workaround however, as we would probably want `ShouldSaveAsZero` not to access `Property` when used the cached path and if we can do without exposing an additional virtual or narrowing down the scope as Matt refers to, that might be the path we take.

Thank you again for the report!

Francis