Fix nullptr dereference in vector_base::fill_insert #6041
Conversation
|
Hi @miscco , Thank you for your guidance on this issue! I assume that the suggested approach of filling the entire vector with x would change the intended behavior of |
thrust/thrust/detail/vector_base.inl
Outdated
| } | ||
|
|
||
| // Handle the simple case: empty vector | ||
| if (size() == 0) |
There was a problem hiding this comment.
This is a rare case that should not need a special case, I believe we should only check for capacity < n and then handle all other cases after that
There was a problem hiding this comment.
Hii @miscco
Done! Now clears existing elements and sets size to 0 when n == 0
updated the code to destroy all elements before returning.
|
/ok to test 08d1ded |
This comment has been minimized.
This comment has been minimized.
|
@bernhardmgruber @miscco |
|
I will let @miscco review this PR next week. |
|
Hi @miscco |
|
/ok to test cb9806f |
| if (size() > 0) | ||
| { | ||
| // we've got room for all of them | ||
| // how many existing elements will we displace? | ||
| const size_type num_displaced_elements = end() - position; | ||
| iterator old_end = end(); | ||
| m_storage.destroy(begin(), end()); | ||
| } | ||
| m_size = 0; |
There was a problem hiding this comment.
Nitpick:
| if (size() > 0) | |
| { | |
| // we've got room for all of them | |
| // how many existing elements will we displace? | |
| const size_type num_displaced_elements = end() - position; | |
| iterator old_end = end(); | |
| m_storage.destroy(begin(), end()); | |
| } | |
| m_size = 0; | |
| if (size() > 0) | |
| { | |
| m_storage.destroy(begin(), end()); | |
| m_size = 0; | |
| } |
| // allocate exponentially larger new storage | ||
| new_capacity = ::cuda::std::max<size_type>(new_capacity, 2 * capacity()); | ||
|
|
||
| // finally, fill the range to the insertion point | ||
| thrust::fill_n(position, n, x); | ||
| } // end if | ||
| else | ||
| { | ||
| // construct new elements at the end of the vector | ||
| m_storage.uninitialized_fill_n(end(), n - num_displaced_elements, x); | ||
| // do not exceed maximum storage | ||
| new_capacity = ::cuda::std::min<size_type>(new_capacity, max_size()); |
There was a problem hiding this comment.
I believe we should rather use ::cuda::std::clamp
| // allocate exponentially larger new storage | |
| new_capacity = ::cuda::std::max<size_type>(new_capacity, 2 * capacity()); | |
| // finally, fill the range to the insertion point | |
| thrust::fill_n(position, n, x); | |
| } // end if | |
| else | |
| { | |
| // construct new elements at the end of the vector | |
| m_storage.uninitialized_fill_n(end(), n - num_displaced_elements, x); | |
| // do not exceed maximum storage | |
| new_capacity = ::cuda::std::min<size_type>(new_capacity, max_size()); | |
| // Ensure allocation grows exponentially wiithin bounds | |
| new_capacity = ::cuda::std::clamp<size_type>(new_capacity, static_cast<size_type>(2 * capacity()), max_size()); |
| if (new_capacity > max_size()) | ||
| { | ||
| throw std::length_error("insert(): insertion exceeds max_size()."); | ||
| } // end if |
There was a problem hiding this comment.
Important: This cannot happen due to the clamp above
| m_storage.destroy(begin(), end()); | ||
|
|
||
| storage_type new_storage(copy_allocator_t(), m_storage, new_capacity); | ||
| // record the vector's new state | ||
| m_storage.swap(new_storage); |
There was a problem hiding this comment.
Nitpick: we should first swap then destroy
| m_size = old_size + n; | ||
| } | ||
| else | ||
| { |
There was a problem hiding this comment.
Nitpick: I would prefer if we would return here and reduce indentation afterwards
| m_size = old_size + n; | |
| } | |
| else | |
| { | |
| m_size = old_size + n; | |
| return; | |
| } | |
| m_storage.destroy(new_storage.begin(), new_end); | ||
| new_storage.deallocate(); | ||
| // finally, fill the range to the insertion point | ||
| thrust::fill_n(position, n, x); |
😬 CI Workflow Results🟥 Finished in 2h 32m: Pass: 98%/70 | Total: 2d 15h | Max: 2h 31m | Hits: 68%/115298See results here. |
3 participants
[BUG FIX] Fix nullptr dereference in vector_base::fill_insert for empty vectors
Summary
This PR fixes a nullptr dereference that occurs when calling fill_insert on an empty
thrust::vectororthrust::device_vector. The issue was that the function would attempt to access begin(), end(), orpositionon unallocated storage.Changes
uninitialized_fill_ndirectly to avoid iterator operations on unallocated storageFixes #2964