Skip to content

Commit

Permalink
Protect from uncaught exception during SHM Segment creation (#3293)
Browse files Browse the repository at this point in the history
* Added regression test

Signed-off-by: Javier Santiago <javiersantiago@eprosima.com>

* Fix Segfault when SharedSegment::compute_per_allocation_extra_size throws

Signed-off-by: Eduardo Ponz <eduardoponz@eprosima.com>

* Refs #16360: Added comments and remove some unneeded code

Signed-off-by: Javier Santiago <javiersantiago@eprosima.com>

* Applied suggestions

Signed-off-by: Javier Santiago <javiersantiago@eprosima.com>

* Uncrustify

Signed-off-by: Javier Santiago <javiersantiago@eprosima.com>

* Fixed Windows warnings

Signed-off-by: Javier Santiago <javiersantiago@eprosima.com>

* Added NOMINMAX flag to test

Signed-off-by: Javier Santiago <javiersantiago@eprosima.com>

---------

Signed-off-by: Javier Santiago <javiersantiago@eprosima.com>
Signed-off-by: Eduardo Ponz <eduardoponz@eprosima.com>
Co-authored-by: Eduardo Ponz <eduardoponz@eprosima.com>
(cherry picked from commit 72d1ba1)

# Conflicts:
#	src/cpp/rtps/DataSharing/WriterPool.hpp
  • Loading branch information
jsan-rt authored and mergify[bot] committed Feb 28, 2023
1 parent 16c3aea commit bf03e48
Show file tree
Hide file tree
Showing 9 changed files with 695 additions and 13 deletions.
17 changes: 9 additions & 8 deletions src/cpp/rtps/DataSharing/DataSharingNotification.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -134,17 +134,18 @@ class DataSharingNotification
{
segment_id_ = reader_guid;
segment_name_ = generate_segment_name(shared_dir, reader_guid);

uint32_t per_allocation_extra_size = T::compute_per_allocation_extra_size(
alignof(Notification), DataSharingNotification::domain_name());
uint32_t segment_size = static_cast<uint32_t>(sizeof(Notification)) + per_allocation_extra_size;

//Open the segment
T::remove(segment_name_);
std::unique_ptr<T> local_segment;

try
{
local_segment = std::unique_ptr<T>(
uint32_t per_allocation_extra_size = T::compute_per_allocation_extra_size(
alignof(Notification), DataSharingNotification::domain_name());
uint32_t segment_size = static_cast<uint32_t>(sizeof(Notification)) + per_allocation_extra_size;

//Open the segment
T::remove(segment_name_);

local_segment.reset(
new T(boost::interprocess::create_only,
segment_name_,
segment_size + T::EXTRA_SEGMENT_SIZE));
Expand Down
45 changes: 44 additions & 1 deletion src/cpp/rtps/DataSharing/WriterPool.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,7 @@ class WriterPool : public DataSharingPayloadPool
writer_ = writer;
segment_id_ = writer_->getGuid();
segment_name_ = generate_segment_name(shared_dir, segment_id_);
<<<<<<< HEAD

// We need to reserve the whole segment at once, and the underlying classes use uint32_t as size type.
// In order to avoid overflows, we will calculate using uint64 and check the casting
Expand Down Expand Up @@ -182,10 +183,52 @@ class WriterPool : public DataSharingPayloadPool

//Open the segment
T::remove(segment_name_);
=======
>>>>>>> 72d1ba171 (Protect from uncaught exception during SHM Segment creation (#3293))
std::unique_ptr<T> local_segment;
size_t payload_size;
uint64_t estimated_size_for_payloads_pool;
uint64_t estimated_size_for_history;
uint32_t size_for_payloads_pool;

try
{
local_segment = std::unique_ptr<T>(
// We need to reserve the whole segment at once, and the underlying classes use uint32_t as size type.
// In order to avoid overflows, we will calculate using uint64 and check the casting
bool overflow = false;
size_t per_allocation_extra_size = T::compute_per_allocation_extra_size(
alignof(PayloadNode), DataSharingPayloadPool::domain_name());
payload_size = DataSharingPayloadPool::node_size(max_data_size_);

estimated_size_for_payloads_pool = pool_size_ * payload_size;
overflow |= (estimated_size_for_payloads_pool != static_cast<uint32_t>(estimated_size_for_payloads_pool));
size_for_payloads_pool = static_cast<uint32_t>(estimated_size_for_payloads_pool);

//Reserve one extra to avoid pointer overlapping
estimated_size_for_history = (pool_size_ + 1) * sizeof(Segment::Offset);
overflow |= (estimated_size_for_history != static_cast<uint32_t>(estimated_size_for_history));
uint32_t size_for_history = static_cast<uint32_t>(estimated_size_for_history);

uint32_t descriptor_size = static_cast<uint32_t>(sizeof(PoolDescriptor));
uint64_t estimated_segment_size = size_for_payloads_pool + per_allocation_extra_size +
size_for_history + per_allocation_extra_size +
descriptor_size + per_allocation_extra_size;
overflow |= (estimated_segment_size != static_cast<uint32_t>(estimated_segment_size));
uint32_t segment_size = static_cast<uint32_t>(estimated_segment_size);

if (overflow)
{
EPROSIMA_LOG_ERROR(DATASHARING_PAYLOADPOOL, "Failed to create segment " << segment_name_
<< ": Segment size is too large: " << estimated_size_for_payloads_pool
<< " (max is " << std::numeric_limits<uint32_t>::max() << ")."
<< " Please reduce the maximum size of the history");
return false;
}

//Open the segment
T::remove(segment_name_);

local_segment.reset(
new T(boost::interprocess::create_only,
segment_name_,
segment_size + T::EXTRA_SEGMENT_SIZE));
Expand Down
18 changes: 15 additions & 3 deletions src/cpp/rtps/transport/shared_mem/SharedMemManager.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -251,9 +251,21 @@ class SharedMemManager :
" characters");
}

uint32_t extra_size =
SharedMemSegment::compute_per_allocation_extra_size(std::alignment_of<BufferNode>::value, domain_name);
return std::shared_ptr<SharedMemManager>(new SharedMemManager(domain_name, extra_size));
try
{
uint32_t extra_size =
SharedMemSegment::compute_per_allocation_extra_size(std::alignment_of<BufferNode>::value,
domain_name);
return std::shared_ptr<SharedMemManager>(new SharedMemManager(domain_name, extra_size));
}
catch (const std::exception& e)
{
EPROSIMA_LOG_ERROR(RTPS_TRANSPORT_SHM, "Failed to create Shared Memory Manager for domain " << domain_name
<< ": " <<
e.what());
return std::shared_ptr<SharedMemManager>();
}

}

~SharedMemManager()
Expand Down
4 changes: 4 additions & 0 deletions src/cpp/rtps/transport/shared_mem/SharedMemTransport.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,10 @@ bool SharedMemTransport::init(
try
{
shared_mem_manager_ = SharedMemManager::create(SHM_MANAGER_DOMAIN);
if (!shared_mem_manager_)
{
return false;
}
shared_mem_segment_ = shared_mem_manager_->create_segment(configuration_.segment_size(),
configuration_.port_queue_capacity());

Expand Down
2 changes: 1 addition & 1 deletion src/cpp/utils/shared_memory/SharedMemSegment.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,7 @@ class SharedSegment : public SharedSegmentBase
}

/**
* Estimates the extra segment space required for an allocation
* Estimates the extra segment space required for an allocation. This may throw
*/
static uint32_t compute_per_allocation_extra_size(
size_t allocation_alignment,
Expand Down
Loading

0 comments on commit bf03e48

Please sign in to comment.