Fix ROOT-7121: Stack corruption in RooVectorDataStore #116
Conversation
| if (!_cache) return ; | ||
|
|
||
| pRealVector tv[1000] ; | ||
| if (static_cast<Int_t>(_cacheUpdateList.size()) < _cache->_nReal) { |
There was a problem hiding this comment.
Could we change _nReal's type to size_t instead?
There was a problem hiding this comment.
This is of course a good idea.
| if (!_cache) return ; | ||
|
|
||
| pRealVector tv[1000] ; | ||
| if (static_cast<Int_t>(_cacheUpdateList.size()) < _cache->_nReal) { |
There was a problem hiding this comment.
I'd check for the capacity of the vector not the size and call reserve instead of resize.
There was a problem hiding this comment.
The way the code worked in the beginning was to directly access the elements of the array. I fixed the array overflow by introducing as few changes as possible: As it works now, reserve won't do the job, it would need more changes further below.
| mutable Double_t _curWgtErr ; // Weight of current event | ||
|
|
||
| RooVectorDataStore* _cache ; //! Optimization cache | ||
| std::vector<RooVectorDataStore::RealVector*> _cacheUpdateList ; //! Pointers to the optimization cache |
There was a problem hiding this comment.
We want to preallocate with reserve() at least 1024 elements, this would reduce the heap reallocations.
|
ping... |
|
Can one of the admins verify this patch? |
|
Hi, I reviewed your suggestions. They are good suggestions, but they are somewhat orthogonal to the fix for the array overflow: Clearly, the current code could use some reworking like getting the types right, but that would require some more work than just the fix of the stack overflow. I could maybe tend to that when I'm back from travelling in May. |
|
Ok, lets check the if the bots are happy with what we have so far. |
|
Can one of the admins verify this patch? |
|
@phsft-bot build! |
|
Starting build on |
|
Clang-format seems unhappy. The other failures are independent on that. |
|
The code looks fine to me. I don't think using vector::reserve is needed in this case. It would be nice to test the performance in a complex fix which takes some time and uses heavily the cache, The tutorial are maybe too simple and the fitting is too fast. Lorenzo |
|
Can one of the admins verify this patch? |
|
Starting build on |
|
Following up on Lorenzo's comment: Can a local vector be used? Long answer: Now, I did timing tests with a heavy workspace, gcc 5.4 -O2. Results are as follows: I therefore committed a version with a local vector and vec.reserve(), the most easy, most clean fix. |
|
@phsft-bot build! |
|
@hageboeck could you fix the formatting issues reported by clang-format (see Details of Travis CI) |
|
Done. I just patched in the diff from the clang-format. |
|
@phsft-bot build! @hageboeck, thanks! |
|
Starting build on |
Fix stack corruption in RooVectorDataStore (ROOT-7121).
4 participants
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