zeromq/zmqpp

receive_raw buffer overflow

Takenbacon opened this issue · 6 comments

Hello, there is currently a buffer overflow issue with receive_raw. If we used the following example code then received a message with a size greater than the buffer's size, it will result in a buffer overflow of buf.

char buf[256];
size_t length; // Note: This is currently only used for returning the size of the message
socket.receive_raw(buf, length);

I suggest two options for resolving this:

  1. (Preferred) Change the implementation to use http://api.zeromq.org/4-1:zmq-recv which has a buffer size argument and will automatically truncate the rest of the message (which is even specified in zmqpp documentation for this function, however it does not work: https://github.com/zeromq/zmqpp/blob/develop/src/zmqpp/socket.hpp#L307)

  2. Change the memcpy to a memmove

I have other minor complaints about how this function is implemented, but they are out of scope of this issue.

Here is the version I am currently using with my application: Takenbacon@34917b7

Some notes about the changes:

  • This is a breaking change
  • Improved performance in ZMQ versions 3.x and higher by removing now useless memcpy
  • Function will now return number of bytes received (0 if error). This is important for application error handling in the event of a truncation
  • Length argument is now used to determine amount of bytes to receive, rather than returning message size
  • This function will now do exactly what is specified in ZMQPP's own header documentation
bluca commented

While I am not familiar with zmqpp, in general I would recommend creating a new API call instead of breaking an existing one. Then, in time, the old one can be deprecated and eventually removed. This is the process we tend to follow with other libraries in the zeromq project, and it avoids creating problems for users and maintainers.

@bluca I thought of that too, however I was honestly just matching the functionality to its own documentation, but yes you're right, I'll make a new API call and fix the invalid documentation. Any suggestions for the name of the new API call? I'm pretty bad at coming up with names.

We could use receive_bytes or something along those lines. then just deprecate this one and encourage switching over.

mm120 commented

I've just hit this same issue. I would suggest fixing the receive_raw function to match it's documentation. This is the code that I have added:
mm120@3983425

I've merged that in as it doesn't break compatibility. It does leave us with the problem that we are silently ignoring the fact we are truncating messages, but I felt that was less of an issue than the potential buffer overflow.

This is what the documentation say, although I can't imagine why I thought that was more sane than reporting an error to the user, so I'm still of the mind we should add separate function that acts that way.