sirupsen/sysvmq

Buffer needs clearing

aerodame opened this issue · 10 comments

I'm having an issue using this GEM where I open a message queue to do some IPC, but whenever I send a new message, it only overwrites the part of the buffer the first time it was sent.

I have C++ versions that work properly for msg_snd, msg_rcv. For example, if I run msg_rcv and then send the string "Hello World" initially, either from this ruby gem or from my C++ code, then send an additional message "FooBar", the resulting message on the receive process will be "FooBarWorld". This points to the buffer not being cleared between each successive message sends.

I can see that your code creates the structure...
typedef struct {
long mtype;
char mtext[];
}

but the array pointer mtext is not initialized but just memcopyied...
...
memcpy(sysv->msgbuf->mtext, RSTRING_PTR(message), blocking.size);

I'm not understanding if a buffer was ever allocated for mtext and I think that is the issue I'm seeing.

My unfortunate workaround at the moment is I need to allocate a 1024 string buffer size and keep clearing it with 1024 zeros between messages sent between two processes.

Any chance you can check into your buffer allocation?

Ah... inspecting my C++ version I think I see the issue clearer. the msg IPC Kernel routines expect this structure....

typedef struct {
long mtype;
char mtext[MSGSZ];
} message_buf;

In other words... a structure with memory allocated to it... not a pointer for mtext.

If you would like my code to msg_snd.cpp... (from my operating systems class at the UW)

///////////////////////////////////////////////////////////
////        File: msg_snd.cpp
//// Description: Demonstrates how to use message queues 
////      Author: Stephen Dame
////        Date: 9-APR-2014
///////////////////////////////////////////////////////////
#include <stdlib.h>  // for exit
#include <stdio.h>   // for perror
#include <sys/ipc.h> // for IPC_CREAT
#include <sys/msg.h> // for msgget, msgrcv
#include <iostream>  // for cin, cout, cerr
#include <string.h>  // for strcpy, strlen
using namespace std;
#define MSGSZ 1024

typedef struct {
   long    mtype;
   char    mtext[MSGSZ];
} message_buf;

int main() {
   int msqid;
   size_t msgsz;
   int msgflg = IPC_CREAT | 0666;
   message_buf mymsg;

   key_t key = 74563;
   if((msqid = msgget(key, msgflg)) < 0) {
      perror("msgget");
      exit(1);
   }

   cout << "Sending to Msg Queue:" << key << endl;

   strcpy( mymsg.mtext, "Hello Huskies!");
   msgsz = strlen(mymsg.mtext) + 1;
   mymsg.mtype = 1;
   if(msgsnd(msqid, &mymsg, msgsz, IPC_NOWAIT) < 0) {
      perror("msgsnd");
   }
}

Hm, this sounds like an architecture difference? We've been running this in production for two years without issues.

I'm currently testing on MacOSX 10.11.2 and Ubuntu 14.04...

Doesn't the kernel message buffer expect when you pass it this message structure...that it has an allocated memory space? For my simple demonstrator above, mtext[MSGSZ] is allocated a hardcoded buffer of 1024.

typedef struct {
   long    mtype;
   char    mtext[MSGSZ];
} message_buf;

In your code, it looks like you have a structure

typedef struct {
   long    mtype;
   char    mtext[];
} message_buf;

Then you have a memcopy that uses that pointer

memcpy(sysv->msgbuf->mtext, RSTRING_PTR(message), blocking.size);

Doesn't this just spew data into the kernel somewhere? That's the effect I'm noticing where I get some random characters at the end of my message on the destination.

I was thinking one quick fix would be to allocate a fixed buffer size of say MSGSZ=8192 and recompile this gem... then deal with a more elegant way to dynamically size message buffers.

(I'm not sure how that works in the real-world --- I've only used msg buffers of fixed size agreement on both sides)

No, since it's a zero-length array—it's size is whatever we malloc it to. This is to have space past the struct of non-hardcoded since, and to avoid having a pointer which would cause a jump in memory.

https://github.com/Sirupsen/sysvmq/blob/master/ext/sysvmq.c#L382

Maybe I'm misunderstanding the spec... but (http://man7.org/linux/man-pages/man2/msgsnd.2.html)
...

       The msgp argument is a pointer to a caller-defined structure of the
       following general form:

           struct msgbuf {
               long mtype;       /* message type, must be > 0 */
               char mtext[1];    /* message data */
           };

       The mtext field is an array (or other structure) whose size is
       specified by msgsz, a nonnegative integer value.  Messages of zero
       length (i.e., no mtext field) are permitted.  The mtype field must
       have a strictly positive integer value.  This value can be used by
       the receiving process for message selection (see the description of
       msgrcv() below).

So, what this spec is saying is that mtext must be an array, not a pointer to an array. Some amount of memory must be allocated to mtext.

Yep, sysvmq does that.

I found a work around. Basically this is a C String message issue. If you see in my C++ message send code, it increments the msgsz by 1 to ensure that the C string null termination is sent in the mtext buffer as well. Without this, a string message will be corrupt on the other receiver end. This works well in your gem as long as I hack in an additional zero and expand the length of the string array.

Snippet of message sending code...

  ...
  str = ""
  str << value.to_s # this ensures it isn't a frozen string
  str << 0          # make sure to pad the value as a C string  
  @mq.send(str)

I guess you can close this issue unless you think the GEM should gracefully handle strings better to make them cross language compatible.

Probably just StringValueCStr after the #to_s, but there is probably some overhead to this. I could put in a PR.

Nevermind, looks like null characters are being expected and that would not work here.