Bug 30658 – Uninitialized memory access in svg SynchronizableProperty hashing

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Matt Mueller]

The hash function in SynchronizablePropertyHash uses sizeof(SynchronizableProperty), however because of struct alignment the memory that is initialized by the constructor is not the entire memory of the struct.  So the hash function will read some extra uninitialized memory after the bool.

This shows up when running various svg layout tests under valgrind, a few examples are: LayoutTests/svg/custom/js-late-mask-and-object-creation.svg, LayoutTests/fast/backgrounds/animated-svg-as-mask.html, LayoutTests/fast/backgrounds/svg-as-background-6.html

In fixing this I noticed that the shouldSynchronize member being used as part of the hash key seems bad, as the hash key should not be mutable.

The attached patch fixes this by removing shouldSynchronize from the hash key and equality comparisons.  After doing this, the code seems a bit like using a HashSet<SynchronizableProperty> to emulate a HashMap<SVGAnimatedPropertyBase*, bool>,  so maybe I should just change it to do that instead?  Not sure if there were other reasons the code was structured this way.

[Matt Mueller]

Created attachment 41630 [details]
Remove shouldSynchronize from hash functions

[Matt Mueller]

Actually it looks like the shouldSynchronize is only used on a per-attribute basis, so it could just be a struct containing a HashSet<SVGAnimatedPropertyBase*> and a shouldSynchronize bit (unless setPropertyNeedsSynchronization can be called before the Properties HashSet is fully populated?)

[Eric Seidel (no email)]

Nikolas wrote this code I think and would be best to comment/review here.

[Darin Adler]

Comment on attachment 41630 [details]
Remove shouldSynchronize from hash functions

If the whole point here is to record a set of properties and a flag for which ones need synchronization, then I suggest using a HashMap<SVGAnimatedPropertyBase*, bool> rather than a HashSet of a new type. Then we could get rid of SynchronizableProperty, SynchronizablePropertyHash, and SynchronizablePropertyHashTraits.

Another option would be to use two HashSet<SVGAnimatedPropertyBase*> -- one for all properties, and another that contains only properties that require synchronization. That could still be done without a custom hash.

>      bool operator==(const SynchronizableProperty& other) const
>      {
> -        return base == other.base && shouldSynchronize == other.shouldSynchronize;
> +        return base == other.base;
>      }

If we don't take my suggestion above, then I think it's poor style to have an "==" operation that will cause two object to compare as equal if some of their state is not identical. I suggest changing SynchronizablePropertyHash::equal instead. I also suggest removing this overload of operator== since it is unlikely there is code that relies on it.

>      static unsigned hash(const SynchronizableProperty& key)
>      {
> -        return StringImpl::computeHash(reinterpret_cast<const UChar*>(&key), sizeof(SynchronizableProperty) / sizeof(UChar));
> +        return StringImpl::computeHash(reinterpret_cast<const UChar*>(&key.base), sizeof(key.base) / sizeof(UChar));
>      }

If we don't take my suggestion above, then since we are hashing just a pointer, it's far better to use PtrHash<SVGAnimatedPropertyBase*>::hash instead of StringImpl::computeHash.

[Darin Adler]

(In reply to comment #2)
> Actually it looks like the shouldSynchronize is only used on a per-attribute
> basis, so it could just be a struct containing a
> HashSet<SVGAnimatedPropertyBase*> and a shouldSynchronize bit (unless
> setPropertyNeedsSynchronization can be called before the Properties HashSet is
> fully populated?)

That sounds good too. Lets do this!

[Matt Mueller]

Created attachment 41909 [details]
Use a HashSet<SVGAnimatedPropertyBase*> and a bool.

[Matt Mueller]

Created attachment 41911 [details]
Use a HashSet<SVGAnimatedPropertyBase*> and a bool.

[Eric Seidel (no email)]

Comment on attachment 41911 [details]
Use a HashSet<SVGAnimatedPropertyBase*> and a bool.

OK.  Queuing for commit.  Commit might take a while as the bots are very behind right now:
http://webkit-commit-queue.appspot.com/

[WebKit Commit Bot]

Comment on attachment 41911 [details]
Use a HashSet<SVGAnimatedPropertyBase*> and a bool.

Clearing flags on attachment: 41911

Committed r50126: <http://trac.webkit.org/changeset/50126>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.