Fix stack corruption in RooVectorDataStore #115
Closed
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a fix for ROOT-7121.
If a cache is updated in RooVectorDataStore and the cache has more than 1000 elements to be updated, an array on the stack will overrun and smash the stack. roofit will therefore crash.
Solution: RooVectorDataStore uses a std::vector instead of an array[1000] to hold the pointers to the cache elements.
Comments on the speed of the fix:
Using a std::vector placed on the stack (mimicking the original implementation), the fits would get slower. Therefore I added the vector as a member of RooVectorDataStore. This saves the time of constantly having to reallocate the vector.
I tested with my (private) workspace: The crash is fixed. Unfortunately, I cannot provide this workspace.
To give a more meaningful test for you guys, I ran all the roofit/roostats tutorials and diffed the logs to check if roofit gives the same results. The diffs are attached. Apart from out-of-order execution and time measurements, there is no difference.
From the time measurements you can also see that the fixed version is not slower.
tutorials_roofit.diff.txt
tutorials_roostats.diff.txt