Bug 30658

Summary: Uninitialized memory access in svg SynchronizableProperty hashing
Product: WebKit Reporter: Matt Mueller <mattm>
Component: SVGAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, eric, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
URL: http://code.google.com/p/chromium/issues/detail?id=25220
Attachments:
Description Flags
Remove shouldSynchronize from hash functions
darin: review-
Use a HashSet<SVGAnimatedPropertyBase*> and a bool.
none
Use a HashSet<SVGAnimatedPropertyBase*> and a bool. none

Matt Mueller
Reported 2009-10-21 19:56:58 PDT
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.
Attachments
Remove shouldSynchronize from hash functions (1.82 KB, patch)
2009-10-21 19:59 PDT, Matt Mueller
darin: review-
Use a HashSet<SVGAnimatedPropertyBase*> and a bool. (10.22 KB, patch)
2009-10-26 16:01 PDT, Matt Mueller
no flags
Use a HashSet<SVGAnimatedPropertyBase*> and a bool. (10.22 KB, patch)
2009-10-26 16:05 PDT, Matt Mueller
no flags
Matt Mueller
Comment 1 2009-10-21 19:59:16 PDT
Created attachment 41630 [details] Remove shouldSynchronize from hash functions
Matt Mueller
Comment 2 2009-10-21 20:11:30 PDT
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)
Comment 3 2009-10-22 10:47:37 PDT
Nikolas wrote this code I think and would be best to comment/review here.
Darin Adler
Comment 4 2009-10-25 15:54:07 PDT
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
Comment 5 2009-10-25 15:55:43 PDT
(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
Comment 6 2009-10-26 16:01:26 PDT
Created attachment 41909 [details] Use a HashSet<SVGAnimatedPropertyBase*> and a bool.
Matt Mueller
Comment 7 2009-10-26 16:05:19 PDT
Created attachment 41911 [details] Use a HashSet<SVGAnimatedPropertyBase*> and a bool.
Eric Seidel (no email)
Comment 8 2009-10-26 17:37:52 PDT
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 9 2009-10-26 20:39:03 PDT
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
Comment 10 2009-10-26 20:39:08 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.