eclipse-iceoryx / iceoryx

Eclipse iceoryx™ - true zero-copy inter-process-communication
https://iceoryx.io
Apache License 2.0
1.6k stars 373 forks source link

Calling clear() on a deque of sample pointers gives error #522

Closed Indra5196 closed 3 years ago

Indra5196 commented 3 years ago

Hi,

I am trying to create a std::deque of sample pointers in order to combine my existing API with iceoryx. (workingset is a private class member of type std::deque) const void* chunk = nullptr; m_subscriber.getChunk(&chunk); auto sample = static_cast<const sample_pointer_type>(chunk); sample_pointer_type ptr(const_cast<sample_pointer_type>(sample)); working_set_.push_back(ptr);

Now, When there is data inside workingset, and I try to call clear() over it, I get free(): invalid pointer

As far as I understand, clear() calls destructor of the deque elements, and since they are NOT heap elements, I get this error. But I am unable to figure out any other way to clear this deque manually.

Please provide clarifications/suggestions/corrections for the same. Thanks in advance

elfenpiff commented 3 years ago

@Indra5196 which iceoryx version are you using? And could you please add the type definition of sample_pointer_type?

Indra5196 commented 3 years ago

@elfenpiff Iceoryx version is 0.17.2.0 sample_pointer_type is a shared_ptr of Struct sample_type which is as follows

sample_pointer_type works exactly like a shared_ptr, just its also associates a bool value with it

struct sample_type { bool flag; iox::cxx::vector<char, 10> charvect; }

elfenpiff commented 3 years ago

@Indra5196 I try to draft your current use case with a hint where I think it goes south.

struct sample_type {
  bool flag;
  iox::cxx::vector<char, 10> charvect;
};

std::deque<sample_type*> working_set_;
void myFunc() {
  const void* chunk = nullptr; 
  m_subscriber.getChunk(&chunk); 
  auto sample = static_cast<const sample_type*>(chunk); 

  // bad:
  // try to avoid const_cast otherwise your program will segfault when you'll try to write into the sample.
  // we made it const since only the publisher is allowed to write into the sample. Just imagine what happens
  // when a second subscriber holds the same sample and reads it while you are writing it, then you enter the
  // land of race conditions and undefined behavior and this can cause really weird and hard to debug bugs.
  sample_pointer_type ptr(const_cast<sample_type*>(sample)); 

  // better alternative:
  const_sample_pointer_type ptr(sample);

  working_set_.push_back(ptr);

  // do stuff

  working_set_.clear(); // XX

2 things are happening here (XX):

  1. free'ing non allocated memory (your bug report) Your sample_pointer_type goes out of scope and calls free since it assumes that the memory was created with malloc. But this is wrong, the memory was created in the shared memory with iceoryx. To fix this you have to options. a) You provide a custom deleter in your sample_pointer_type which calls m_subscriber.releaseChunk() b) You do not use a sample_pointer_type at all.

  2. memory leak in the shared memory and in iceoryx. Every memory you acquire with iceoryx via getChunk has to be free'd with iceoryx mechanisms. Therefore you have to call releaseChunk otherwise you are having a memleak inside the shared memory and after a while getChunk will print error messages to the console and will return nullptr.

I hope I could help you. With the new typed API, which is in an experimental state in v0.9.0, the issue should be gone since we are returning there a sample_ptr which works also like a smart_ptr. You could try it out and give us some feedback, maybe we can still improve it in one way or another.

Indra5196 commented 3 years ago

As of now, I was able to solve this issue by creating the shared_ptr in sample_pointer_type with a blank deleter

I have to use sample_pointer_type in order to keep our current API intact. As of now, there is no way around that for me

And yes, I am calling releaseChunk after workingset.push_back()

mossmaurice commented 3 years ago

Closing this issue. @Indra5196 Feel free to re-open.