Fix buffer overflow in DWI bootstrapping #2779
Conversation
Fix applies to tckgen -algorithm tensor_prob.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@MRtrix3/mrtrix3-devs:
|
|
As mentioned in #2745, we should discuss which clang-tidy checks we should enable as some of them may be either undesirable or superfluous in the context of our codebase. However, it's strange that it's complaining about |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
After fixing merge conflicts to get this to |
- Different syntax for adding elements to std::map<> Bootstrap::voxels seems to prevent meory leaks; it seems that using std::make_pair<> to add elements is somehow inappropriate. - Re-introduce tracking of chunk index that was erroneously removed in 7fa0142, but with an additional fix: - If previous chunks have previously been allocated, those memory allocations are preserved. Therefore, for a new streamline, once a chunk has been fully utilised, the next pre-allocated chunk should then be used. This was misunderstood in the generation of 7fa0142. - Includes a fix for this intended behaviour. Previously, once transitioning to a new chunk, value_type *next_voxel would erroneously point to the start of the final chunk, rather than the start of the next chunk. This is rectified by this commit. - Additionally, allocate the first chunk of memory in the constructor. While this does not resolve any specific memory issue, it IMO more clearly conveys where and why memory chunks are allocated.
|
clang-tidy review says "All clean, LGTM! 👍" |
| voxel_buffer.push_back(std::vector<value_type>(NUM_VOX_PER_CHUNK * size(3))); | ||
| assert(current_chunk < voxel_buffer.size()); | ||
| next_voxel = &voxel_buffer.back()[0]; | ||
| next_voxel = &voxel_buffer[current_chunk][0]; |
There was a problem hiding this comment.
@jdtournier Note bug here resolved in b2fa527.
|
OK, this now passes tests with address sanitiser. |
Two components to change:
Removal of
current_chunkshould have no effect. It looks to me like vestigial code from trying different approaches to keep track of buffer chunks, but the use ofnext_voxel&last_voxelsatisfies the requirement.While
get_voxel()looks like it's doing exactly the same thing, though perhaps less elegantly, it nevertheless resolves a buffer overflow. Though it looks to me like an invalid or overwritten address rather than a conventional "off-by-one"-type overflow.On
dev, within an individual thread, thevalue_type*yielded by the map for a previously sampled voxel is occasionally different to that used when buffer data were allocated for that voxel. Maybe one in every 1000 times that a pointer for a previously sampled voxel is requested.I can't report exactly what is wrong with the
devcode. If it were possible for thestd::mapimplementation to perform an internal tree rebalancing after it has done its search as part ofinsert(), but the yielded pointer address pointer were that from before rather than after the rebalancing, that would seem to me like a pretty large STL implementation bug.