[LIBZMQ-450] Copy the stream engine endpoint - string reference caused memory corruption #473

Merged
merged 1 commit into from Nov 16, 2012

Projects

None yet

4 participants

@methodmissing
The ZeroMQ project member

References https://zeromq.jira.com/browse/LIBZMQ-450 :

"Adrien Kunysz added a comment - 16/Nov/12 11:45 AM
As per previous comment. I am still able to reproduce the issue after applying the patch."

@ianbarber ianbarber merged commit 1a18c7b into zeromq:master Nov 16, 2012
@ianbarber
The ZeroMQ project member

Looks good, but might be an idea to replace the strcpy with strncpy

@hurtonm
The ZeroMQ project member

I think this does not solve the problem. The whole engine, along with the address buffer, can be deallocated before the user app can read the notification message. I would suggest to copy the address into that notification message.

@methodmissing methodmissing added a commit to methodmissing/libzmq that referenced this pull request Nov 17, 2012
@methodmissing methodmissing Revert "Merge pull request #473 from methodmissing/fix-engine-endpoint"
This reverts commit 1a18c7b, reversing
changes made to bef9a41.
ce4d321
@methodmissing
The ZeroMQ project member

Hi Martin,

Reworked in https://github.com/methodmissing/libzmq/compare/copy-monitor-endpoints

  • Reverted the merge commit for this PULL
  • Prefer std::string address types as arguments to internal event callbacks
  • Copies endpoint addresses to zmq_event_t structs passed as messages

Thoughts ?

@hurtonm
The ZeroMQ project member

You are allocating memory using new, so the consumer should use [] delete to release the memory. What if the application uses C or Java binding? Also, when the application closes the notification socket, it needs to be sure there is no message ready to be received. Otherwise the application will leak memory.
What about using multi-part messages to transfer events - one message part per field of notification structure.
Another option is to embed a fixed size array in the event structure and pass copy of endpoint address rather than pointer to copy of endpoint address. The obvious difficulty is how to choose the size of that array.
Perhaps we should move the discussion to ML.

@hintjens
The ZeroMQ project member
@methodmissing
The ZeroMQ project member

Hi Guys,

I had time to briefly scan Martin's feedback before boarding for a flight yesterday and spent that time extracting a symmetric message oriented API that requires the consumer to explicitly free any events :

  • methodmissing@b34684d
  • zmq_event_recv - similar to zmq_msg_recv, but populates a zmq_event_t struct
  • zmq_event_close - similar to zmq_msg_close, frees any allocated metadata ( endpoint address on most structs )
  • This is as close to a familiar messaging API as I could get, however it'll still leak if zmq_socket_monitor is called for a socket without a consuming PAIR socket
  • A few folks have asked for binding examples / implementations - this works well against current master for Ruby, which has a GIL like Python and some other scripting languages methodmissing/rbczmq@0b4a7d9
  • Not able to reproduce any failures on OS X ( my primary work environment - I could never reproduce any of the reported event specific failures, it seems a lot more deterministic than other platforms ) or Ubuntu

Shall I summarize recent changes for context and we resume this discussion on the ML ?

@hurtonm
The ZeroMQ project member

Another option is to initialise event messages using msg_t::init_data method. That way, you may create a temporary buffer in the library holding the endpoint's address and pass a function that frees that buffer to init_data. Once the user closes the received event message, the library will free the buffer's memory. This way we do not need to extend the current API.

@hurtonm
The ZeroMQ project member

As for failures, it depends on memory allocator in use, I think.

@methodmissing
The ZeroMQ project member

good point

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment