CODEWITHSUNDEEP
csockets
Cameron
Cameron

Reputation: 13

Socket data corruption

I'm working on a simple socket client, that sends a simple letter "p" to the server, and then reads the response from the server. It is working fully, except for one confusing issue. The very first time the socket is read from (it happens in a loop), the data is garbled and corrupt, with results like "ÿýÿû" and "µÞv". All the data received after the first response is fine and valid.

The code I'm using to receive is:

int n;
char buffer[256];
bzero(buffer,256);
strcpy(buffer, "p");
n = write(manSock,buffer,256);
if (n < 0)
{
 error("ERROR writing to management server");
}
bzero(buffer,256);
n = read(manSock,buffer,256);
if (n < 0)
{
 error("ERROR reading from management server");
}
return buffer;

manSock is the socket file descriptor.

Any ideas on why this is happening?

Upvotes: 0

Views: 2151

Answers (4)

user207421
user207421

Reputation: 310860

You've designed your API completely incorrectly

  1. You're returning the address of a local buffer, which won't exist and may be overwritten after the call.
  2. You're throwing away the length returned by recv(), so the caller has no way of knowing how much of the buffer is valid, even if you fixed (1) somehow.

You need the caller to supply the buffer, and you need to return the length. This makes your method signature look remarkably like that of recv(). In Other words you possibly don't really need this method at all. The caller could just call recv().

Upvotes: 0

alk
alk

Reputation: 70893

This does not seem to be a problem related to sockets, but to memory management.

You seem to return a pointer to memory being valid only local to the function.

Assuming your "real" code looks like this

char * foo(void)
{
  char buffer[256];

  /* read into buffer */

  return buffer;
}

void bar (void)
{
  char * p = foo();
  printf("%s\n", p);
}

then p refers to invalid memory after foo() returned, as buffer had been implicitly deallocated upon foo()'s return.

To fix this

  • either allocate buffer dynamical in foo() using malloc(), calloc() or strdup()

    char * foo(void)
{
  char * buffer = malloc(256);
  memset(buffer, 0, 256);
  /* read into buffer */
  return buffer;
}

    or

    char * foo(void)
{
  char * buffer = calloc(256, sizeof(*buffer));
  /* read into buffer */
  return buffer;
}

    or 

    char * foo(void)
{
  char buffer[256] = {0};
  /* read into buffer */
  return strdup(buffer);
}

  • or pass down to foo() a reference to a buffer being allocated in bar() (or higher).

    void foo(char * buffer)
{
   /* read into where buffer points */
}

void bar(void)
{
  char buffer[256] = {0};
  foo(buffer);
  /* print buffer */
}

Upvotes: 1

dvasanth
dvasanth

Reputation: 1377

You need to send only upto the length(strlen) of the buffer. Its best practice to always send the length of the buffer before actually sending the data.

int len;
len = strlen(buffer);
n = write(manSock,&len , sizeof(int));
if (n < 0)
{
 error("ERROR writing to len management server");
}
n = write(manSock,buffer,strlen(buffer));
if (n < 0)
{
 error("ERROR writing to management server");
}
bzero(buffer,256);
n = read(manSock,&len,sizeof(int));
if (n < 0)
{
  error("ERROR reading len from management server");
}
n = read(manSock,buffer,len);
if (n < 0)
{
  error("ERROR reading from management server");
}

Upvotes: 0

PaulMcKenzie
PaulMcKenzie

Reputation: 35440

Your code as it stands now (assuming it is a function) is bad, even if you took the advice given in the other answers and comments concerning the write and read functions.

The reason is that you're returning a locally defined array (actually the address of the first element), buffer, and doing so is undefined behavior. So if you're retrieving the return value, buffer is no longer valid and could conceivably contain garbage data, even if you filled it with valid data within the function.

The C++ tag was removed, but if you really are using C++, you could always copy the results to a std::string and return that instead:

std::string someFunc()
{
    int n;
    char buffer[256];
    //.. your code goes here
    //...
    return std::string(buffer, len); // where len is the number of characters read
}

If you're using C, then have the user pass you the buffer and you fill it in within the function. Don't create local arrays and return them -- that's the bottom line.

Upvotes: 0

Related Questions