Segmentation fault on TCP recv

Hi everyone,

I have run into an issue where I need to transmit a custom struct I created to a program using TCP sockets and threads.

I believe that I am allocating memory correctly but I receive a seg fault every time the recv() call is reached. So I am not really sure what is going on.

Code:

        //struct definition
	typedef struct
	{
		char *header;
		char data[256];
	}Message;

   void * respond(void * incomingSocketFD)
   {
        printf("in respond\n");
        char buffer[32];
        int newFD;
	nt retval;
		
	//copy socket 
	memcpy(&newFD, &incomingSocketFD, sizeof(incomingSocketFD));
		
	//allocate space for message
	message = malloc(sizeof(Message *));
   	message->header = (char *)malloc(sizeof(char));
		
      while(retval != 0)
      {
	printf("inside while in respond\n");
      	/* Receive data from the client */
         retval = recv(newFD, message, sizeof(Message), 0); //!!!!!SEG FAULT HERE

         if(retval < 0)//error check
         {
            perror("NATbox:recv()");
            exit(1);
         }
       }//end while
}//end respond

Any help/pointers would be great,
Thanks,
Hunter

> //copy socket
> memcpy(&newFD, &incomingSocketFD, sizeof(incomingSocketFD));
Tell me, is this a thread function?
Show us how you invoke this thread - in particular, details of the thread parameter pointer. Is this a pointer to a local variable (this is almost always a bug).

Some points,
1. the size you're copying is sizeof(void*), not the size of an integer
2. &incomingSocketFD is a pointer to the parameter, not where it points to (drop the &)

> message->header = (char *)malloc(sizeof(char));
What is the point of allocating a single char, when you have a fixed sized array also in the struct?

> retval = recv(newFD, message, sizeof(Message), 0);
You just trashed your previous malloc.

Presumably message is a pointer to a Message. You should allocate sizeof(Message), not sizeof(Message *). You want enough bytes to store all the elements of the Message structure, not just 4 bytes for a pointer. The safest way to do this is:

Code:

message = malloc(sizeof(*message));

You also have the first element of the Message structure declared as a char *. If you try to read from the socket into that, you wont get what you think. The header pointer on the sending system will not be valid on the receiving system. You allocate space for it, but you only allocate sizeof(char), which is one byte. Also, that byte you allocated doesn't live at the beginning of the struct. It lives somewhere else, and is only pointed to by the address in header. The same goes for your sending code. You probably want to make header a fixed-size array like data, or if you have to allocate memory, allocate sufficient space and read in chunks:

Code:

message->header = malloc(42);  // note I don't cast malloc

recv(newFD, message->header, 42, 0);
recv(newFD, message->data, sizeof(message->data), 0);

Lastly, if you are talking across different architectures or compilers, you may find your send and receive structures don't match up due to padding or alignment issues.

You've used sizeof(message*) when allocating memory... Most likely that got you 4 bytes. Try it without the *, like you did in recv.

You did the same when allocating a single character to your message header.

Then you went totally rogue and asked recv ... which expects a byte oriented buffer to load your message struct directly.

In response to your question Salem, yes this is a thread function, I have included the portion of code where I create the thread and start the function, including the argument I pass the function.

Code:

 /*
   * Recv the data from the sender
   */
      while (1)
      {
      /* accept a connection from a client program */
         int newFD;
         newFD = accept( socketFD, (struct sockaddr *)&srcAddr, (socklen_t *)&fromlen );
         if ( newFD < 0 )
         {
            perror( "NATbox:accept()" );
            return -1;
         }
      
         int threadError = pthread_create(&tid, NULL, respond, (void *) newFD); 
         if(threadError < 0)
         {
            perror("NATbox:spawn thread\n");
            return -1;
         }
      }
		
      int status;
      int i = 0;
      while( i != -1 ) 
      {
         i = wait( &status );
         printf( "Finished pid = %d\n", i );
      }
   
   /*
   * Return a 0 to say it worked correctly from this end
   */
      return 0;
   }
	
   void * respond(void * incomingSocketFD)
   {
      printf("in respond\n");
      char buffer[32];
      int newFD;
      int retval;
		
      //copy socket 
	memcpy(&newFD, &incomingSocketFD, sizeof(incomingSocketFD));
		
	//allocate space for message
	message = malloc(sizeof(Message));
   	message->header = (char *)malloc(sizeof(char *));
		
      while(retval != 0)
      {
	printf("inside while in respond\n");
      	/* Receive data from the client */
         retval = recv(newFD, message, sizeof(Message), 0);

         if(retval < 0)//error check
         {
            perror("NATbox:recv()");
            exit(1);
         }

I changed the struct as you suggested to have the header field be a fixed array of size 1, which along with your suggested changes to my malloc call have resolved my segmentation fault issue.

The header field of the Message struct is just a single character which indicates what service the client program wants, "A", "M", or "C".

Thanks for the help.
Hunter

Originally Posted by mcmillhj

The header field of the Message struct is just a single character which indicates what service the client program wants, "A", "M", or "C".

I hope you meant 'A', 'M' or 'C', since those are character literals. The double " " is for string literals, meaning they

1. Take up two bytes (one for the letter, one for the terminating '\0')
2. Can't be assigned with = or compared with ==, you need strcpy and strcmp.

That is what I meant, thanks for pointing that out, one of my Java habits I suppose.

Hunter

Well your thread seems OK, if a little unconventional (and assuming pointers and ints are the same size).

But you're still allocating memory, and then overwriting your allocation.

Can you show us the send code as well?
I hope you're not trying to send that structure "as is", complete with a pointer.

The send is working fine, I am reluctant to post the send code as this is for a class project, that is why I only posted a segment of the recv code.

Hunter

Code:

	typedef struct
	{
		char *header;
		char data[256];
	}Message;
        Message msg;

If you do
send(fd,&msg,sizeof(msg),0);
then your send is BROKEN.
The 4 bytes of the pointer will arrive OK, but it will be completely meaningless to the receiver. send() certainly isn't going to dereference the pointer for you (how would it know) and send what it points to.

You need to do at least
send(fd,msg.header,strlen(msg.header)+1,0);
send(fd,msg.data,strlen(msg.data)+1,0);
This sends both strings, AND a separating \0.

If your header/data are binary data, then send a length value in advance of the data itself.

my send call looks like this:

Code:

send(fd, message, sizeof(message), 0);
//message is a pointer to a struct of type Message

Is that wrong?

Hunter

Originally Posted by mcmillhj

my send call looks like this:

Code:

send(fd, message, sizeof(message), 0);
//message is a pointer to a struct of type Message

Is that wrong?

Hunter

If struct message includes pointers, per the definition you published above, yes it's wrong.

What happens is that the pointer may be valid on the sending system but it is the pointer itself, not the data at the pointer that is transferred. On the receiving end attempting to access the pointer, which is not valid on that machine, causes a seg-fault.

You explained that your header is a single character, so...

Code:

typedef struct tMessage
{  char header;
    char data[256]; }
Message, *pMessage;

Message msg;

That should fix the problem.

> //message is a pointer to a struct of type Message
Yes, it's wrong twice.
1. You use sizeof() on what appears to be a pointer, not what it points at.
2. What it points at in itself also contains pointers.

Check the return values of send()
Does it look like you've sent two complete strings?

I have redefined my struct so that it contains two members, a character, and a fixed size array of characters. You are correct about me only transferring sizeof(message) which was not sending the whole struct, just the header character.

Code:

typedef struct
{
    char header;
    char data[256];
}Message;

//new send call
send(fd, message, sizeof(Message), 0);

Does that look better? It now sends the whole struct and my server is responding correctly.

Thanks
Hunter

Originally Posted by mcmillhj

Does that look better? It now sends the whole struct and my server is responding correctly.

Then, yes it looks better...