elBoberido
commented
1 week ago The only way I can see this happening is if m_usedListHead is modified between the start of the loop and the later if check. Is that possible?
Are you accessing the endpoint (publisher, subscriber, client, server) from multiple threads? I thinks this is the only way for the scenario you describe. The endpoints are not thread safe and you need to use e.g. a smart_lock with them. You can use multiple endpoints with the same ServiceDescription in multiple threads but not the same instance of an endpoint across multiple threads without a mutes.
tobiasstarkwayve
Required information
Operating system:
Compiler version: Clang 16
Eclipse iceoryx version: v. 2.0.3 (but code is unchanged since then)
Observed result or behaviour: As we were tracking down a segmentation fault we added the following assertions into the used-chunks list:
Diff
``` ✔ ~/iceoryx [v2.0.3|✚ 2…3] 12:25 $ git diff diff --git a/iceoryx_posh/include/iceoryx_posh/internal/popo/building_blocks/chunk_receiver_data.hpp b/iceoryx_posh/include/iceoryx_posh/internal/popo/building_blocks/chunk_receiver_data.hpp index 01e1c9ac4..0bcb35975 100644 --- a/iceoryx_posh/include/iceoryx_posh/internal/popo/building_blocks/chunk_receiver_data.hpp +++ b/iceoryx_posh/include/iceoryx_posh/internal/popo/building_blocks/chunk_receiver_data.hpp @@ -44,6 +44,9 @@ struct ChunkReceiverData : public ChunkQueueDataType /// has to return one to not brake the contract. This is aligned with AUTOSAR Adaptive ara::com static constexpr uint32_t MAX_CHUNKS_IN_USE = MaxChunksHeldSimultaneously + 1U; UsedChunkList m_chunksInUse;
+
+private:
+ void self_check();
};
} // namespace popo
diff --git a/iceoryx_posh/include/iceoryx_posh/internal/popo/used_chunk_list.inl b/iceoryx_posh/include/iceoryx_posh/internal/popo/used_chunk_list.inl
index eb867880c..310317610 100644
--- a/iceoryx_posh/include/iceoryx_posh/internal/popo/used_chunk_list.inl
+++ b/iceoryx_posh/include/iceoryx_posh/internal/popo/used_chunk_list.inl
@@ -38,9 +38,11 @@ UsedChunkList::UsedChunkList() noexcept
template
bool UsedChunkList::insert(mepoo::SharedChunk chunk) noexcept
{
+ self_check();
auto hasFreeSpace = m_freeListHead != INVALID_INDEX;
if (hasFreeSpace)
{
+ iox::cxx::Ensures(m_freeListHead < INVALID_INDEX);
// get next free entry after freelistHead
auto nextFree = m_listIndices[m_freeListHead];
@@ -48,17 +50,20 @@ bool UsedChunkList::insert(mepoo::SharedChunk chunk) noexcept
m_listIndices[m_freeListHead] = m_usedListHead;
m_usedListHead = m_freeListHead;
+ iox::cxx::Ensures(m_usedListHead < INVALID_INDEX);
m_listData[m_usedListHead] = DataElement_t(chunk);
// set freeListHead to the next free entry
m_freeListHead = nextFree;
+ self_check();
/// @todo can we do this cheaper with a global fence in cleanup?
m_synchronizer.clear(std::memory_order_release);
return true;
}
else
{
+ self_check();
return false;
}
}
@@ -67,10 +72,11 @@ template
bool UsedChunkList::remove(const mepoo::ChunkHeader* chunkHeader, mepoo::SharedChunk& chunk) noexcept
{
auto previous = INVALID_INDEX;
-
+ self_check();
// go through usedList with stored chunks
for (auto current = m_usedListHead; current != INVALID_INDEX; current = m_listIndices[current])
{
+ iox::cxx::Ensures(current < INVALID_INDEX);
if (!m_listData[current].isLogicalNullptr())
{
// does the entry match the one we want to remove?
@@ -85,6 +91,7 @@ bool UsedChunkList::remove(const mepoo::ChunkHeader* chunkHeader, mepo
}
else
{
+ iox::cxx::Ensures(previous < INVALID_INDEX);
m_listIndices[previous] = m_listIndices[current];
}
@@ -94,11 +101,13 @@ bool UsedChunkList::remove(const mepoo::ChunkHeader* chunkHeader, mepo
/// @todo can we do this cheaper with a global fence in cleanup?
m_synchronizer.clear(std::memory_order_release);
+ self_check();
return true;
}
}
previous = current;
}
+ self_check();
return false;
}
@@ -115,7 +124,7 @@ void UsedChunkList::cleanup() noexcept
data.releaseToSharedChunk();
}
}
-
+ self_check();
init(); // just to save us from the future self
}
@@ -147,9 +156,19 @@ void UsedChunkList::init() noexcept
data.releaseToSharedChunk();
}
+ self_check();
m_synchronizer.clear(std::memory_order_release);
}
+template
+void UsedChunkList::self_check() noexcept {
+ for (uint32_t i = 0U; i < Capacity; ++i) {
+ iox::cxx::Ensures(m_listIndices[i] <= INVALID_INDEX);
+ }
+ iox::cxx::Ensures(m_freeListHead <= INVALID_INDEX);
+ iox::cxx::Ensures(m_usedListHead <= INVALID_INDEX);
+}
+
} // namespace popo
} // namespace iox
```
Running with this patch, we observed a violation of the check ahead of the
m_listIndices[previous] = m_listIndices[current];line. This seems like a clear bug to me, as it implies an out-of-bounds access on the array. Do you agree?Furthermore, it looks to me like this assertion can be triggered only if the else branch is taken in the initial iteration of the loop (as any subsequent iteration will have previous < INVALID_INDEX as ensured by the loop condition). The only way I can see this happening is if m_usedListHead is modified between the start of the loop and the later if check. Is that possible? If so, does the code properly handle that case? Wouldn't this need some sort of retry-loop?
Expected result or behaviour: No out-of-bounds accesses
Conditions where it occurred / Performed steps:
The bug is hard to reproduce, but on our system it appears reliably after a few thousand runs of our periodic pipeline. Changing the size of the images being processed affects whether the problem occurs (?), which we assume is due to higher load on the system
Additional helpful information