unix domain stream sockets sending more data then it should be

Question

I have two simple programs set up that share data through a unix domain socket. One program reads data out of a Queue and sends it to the other application. Before it is sent each piece of data is front-appended by four bytes with the length, if it is less then four bytes the left over bytes are the '^' symbol.

The client application then reads the first four bytes, sets a buffer to the appropriate size and then reads the rest. The problem that I'm having is that the first time through the message will be sent perfectly. Every other time after that there is extra data being sent so a message like "what a nice day out" would come out like "what a nice day out??X??". So I feel like a buffer is not being cleared correctly but I can't seem to find it.

Client code:

listen(sock, 5);
for (;;) 
{
    msgsock = accept(sock, 0, 0);
    if (msgsock == -1)
        perror("accept");
    else do 
 {
  char buf[4];
        bzero(buf, sizeof(buf));
        if ((rval = read(msgsock, buf, 4)) < 0)
        perror("reading stream message");

  printf("--!%s\n", buf);

  string temp = buf;
  int pos = temp.find("^");
  if(pos != string::npos)
  {
   temp = temp.substr(0, pos);
  }

  int sizeOfString = atoi(temp.c_str());
  cout << "TEMP STRING: " << temp << endl;
  cout << "LENGTH " << sizeOfString << endl;
  char feedWord[sizeOfString];
  bzero(feedWord, sizeof(feedWord));

  if ((rval = read(msgsock, feedWord, sizeOfString)) < 0)
              perror("reading stream message");

          else if (rval == 0)
              printf("Ending connection\n");
          else
              printf("-->%s\n", feedWord);
     bzero(feedWord, sizeof(feedWord));
        sizeOfString = 0;
     temp.clear();
      } 
  while (rval > 0);
      close(msgsock);
  }
  close(sock);
  unlink(NAME);

Server Code

                pthread_mutex_lock(&mylock);
    string s;
    s.clear();
    s = dataQueue.front();
    dataQueue.pop();
    pthread_mutex_unlock(&mylock);

    int sizeOfString = strlen(s.c_str());
    char sizeofStringBuffer[10];

    sprintf(sizeofStringBuffer, "%i", sizeOfString);
    string actualString = sizeofStringBuffer;
    int tempSize = strlen(sizeofStringBuffer);

    int remainder = 4 - tempSize;
    int x;
    for(x =0; x < remainder; x++)
    {
     actualString = actualString + "^";
    }

    cout << "LENGTH OF ACTUAL STRING: " << sizeOfString << endl;

    actualString = actualString + s;

    cout << "************************" << actualString << endl;
    int length = strlen(actualString.c_str());

    char finalString[length];
    bzero(finalString, sizeof(finalString));
    strcpy(finalString, actualString.c_str());

                        if (write(sock, finalString, length) < 0)
                        perror("writing on stream socket");

Answer 1

Rather than padding your packet length with '^', you'd be far better off just doing:

snprintf(sizeofStringBuffer, 5, "%04d", sizeOfString);

so that the value is 0 padded - then you don't need to parse out the '^' characters in the receiver code.

Please also edit out your debug code - there's only one write() in the current code, and it doesn't match your description of the protocol.

Ideally - split your sending routine into a function of its own. You can also take advantage of writev() to handle coalescing the string holding the "length" field with the buffer holding the actual data and then sending them as a single atomic write().

Untested code follows:

int write_message(int s, std::string msg)
{
     struct iovec iov[2];
     char hdr[5];

     char *cmsg = msg.c_str();
     int len = msg.length();

     snprintf(hdr, 5, "%04d", len);  // nb: assumes len <= 9999;

     iov[0].iov_base = hdr;
     iov[0].iov_len = 4;

     iov[1].iov_base = cmsg;
     iov[1].iov_len = len;

     return writev(s, iov, 2);
}

Answer 2

You have to check return values of both write and read not only for -1 but for short (less then requested) writes/reads. You also seem to just continue after printing an error with perror - do an exit(2) or something there.

Answer 3

Two things:

First - on the Server side you are writing off the end of your array.

char finalString[length];
bzero(finalString, sizeof(finalString));
strcpy(finalString, actualString.c_str());

The strcpy() will copy length+1 characters into finalString (character pull the null terminator).

Second (and most likely to be the problem) - on the client side you are not null terminating the string you read in, therefore the printf() will print your string, and then whatever is on the stack up to the point it hits a null.

Increase both buffers by one, and you should be in better shape.