[subset] null substitutions in FeatureVariationRecord trips assert in assign_offset()

[blueshade7]

Found this issue while investigating https://crbug.com/oss-fuzz/21560

While serializingFeatureVariationRecord with non-nullconditions and null substitutions, subset() adds links for conditions then fails because of null substitutions. As a recovery attempt, the code below calls hb_serialize_t::revert() to restore the serialize buffer, as the result the same address in the serialize buffer may be reused for another link to a condition in the next FeatureVariation.

harfbuzz/src/hb-ot-layout-common.hh

Lines 245 to 251 in c8cc1e3

	operator () (T&& record)
	{
	auto snap = subset_layout_context->subset_context->serializer->snapshot ();
	bool ret = record.subset (subset_layout_context, base);
	if (!ret) subset_layout_context->subset_context->serializer->revert (snap);
	else out->len++;
	}

Multiple links at the same address eventually triggers an assert in hb_serialize_t::assign_offset():

harfbuzz/src/hb-serialize.hh

Lines 485 to 491 in c8cc1e3

	template <typename T>
	void assign_offset (const object_t* parent, const object_t::link_t &link, unsigned offset)
	{
	auto &off = * ((BEInt<T, sizeof (T)> *) (parent->head + link.position));
	assert (0 == off);
	check_assign (off, offset);
	}

I believe FeatureVariationRecord::sanitize() should reject null substitutions. Currently it calls the generic OffsetTo::sanitize() which accepts null.

[ebraminio]

Cc: @garretrieger @qxliu76

[blueshade7]

Another way to see this problem is that revert () does not undo links added since snap ().

[qxliu76]

Just a guess, we may need add push () and pop_discard () pair after snap() and revert ().
I'll work on this.

[qxliu76]

should be fixed with #2315

[behdad]

Another way to see this problem is that revert () does not undo links added since snap ().

That sounds like the real problem to me.

[behdad]

I believe FeatureVariationRecord::sanitize() should reject null substitutions. Currently it calls the generic OffsetTo::sanitize() which accepts null.

I disagree with this.

[behdad]

Another way to see this problem is that revert () does not undo links added since snap ().

I think is obvious to me now. snap/revert only recycle full objects. We want snap to also remember number of links in current object and revert that as well. Is that what @blueshade7 you think as well?

[blueshade7]

We want snap to also remember number of links in current object and revert that as well.

That seems a natural behavior of snap()/revert() pair to me.

[behdad]

I'm actually not sure. If susbstitions is empty we should return false and those objects cleaned. Let me dig more.

[behdad]

Okay I see. The record lives dangling links in the current object indeed because records are part of the parent. Making revert clean up is indeed one way. Let me think of implications / alternatives.

[behdad]

Normally when a subset call returns false it doesn't need to clean up after itself because the offset object that calls it cleans up. But here the subset returning false is part of a bigger object, so the links don't get cleaned up. Let me think which direction I like to go with a fix.

[behdad]

Okay yes I think I like to adjust snapshot/revert to revert links in current object as well. That fully clarifies the issue.

[behdad]

I believe FeatureVariationRecord::sanitize() should reject null substitutions. Currently it calls the generic OffsetTo::sanitize() which accepts null.

Actually that's a very valid usecase and legal. See #2334

[blueshade7]

Actually that's a very valid usecase and legal. See #2334

Probably. What I don't like about OT spec is that it sometimes says "offset may be NULL", then it must mean that the subtable may be legally omitted by being NULL. However in many cases it doesn't say including this one. Possible interpretations are:

1. The same as "offset may be NULL" (then why says it sometimes, not other times?)
2. NULL offset is illegal (my interpretation)
3. Take zero offset literally, i.e., reinterpret the beginning of the parent table as the subtable (crazy, but a possible interpretation)

Spec must be unambiguous.

[behdad]

@blueshade7 anything to do here?

[blueshade7]

I don't know; I noticed oss-fuzz/21560 has been "fixed" maybe by other means? We can't debug & verify its fix without a reproducible test case anyways. If it's not fixed, I imagine fuzzer should rediscover it for us.:shrug:

[behdad]

Another way to see this problem is that revert () does not undo links added since snap ().

This has indeed been fixed.