• Status: Solved
  • Priority: Medium
  • Security: Public
  • Views: 2625
  • Last Modified:

Errors in reading a binary file from socket - C for linux

I'm writing a simple Client program in C using sockets to transfer a file from a server also written in C which is running ( and tested correctly). The problem is, I'm able to transfer text or ASCII files from it, but not the binary files like images etc. Though after sending an mp3 file, my music player was able to play it, but I guess it must be the player's capabilities rather than my program's correctness - because i know that at least jpg and bmp files are not correctly being displayed ( actually not displayed at all :) )

I don't understand what wrong I'm doing here. Can somebody tell me?



//Here's the code:







#include<stdio.h>
#include<sys/types.h>
#include<sys/socket.h>
#include<netinet/in.h>
#include<netdb.h>
#include<stdlib.h>
#include<string.h>
#include<unistd.h>
 
#define BUFFER_LENGTH 256
 
 
//Function Declarations:
unsigned long findFileSize(char* header);
char* removeHeader(char *string);
void error(char *msg);
 
 
 
int main(int argc, char*argv[])
{
	int s;
	int portnum;
	struct sockaddr_in sa;
	struct hostent *server;
	int n;
	char buffer[BUFFER_LENGTH], buffer2[BUFFER_LENGTH];
	char filename[100];
	unsigned long fileSize=0, sizeAlreadyRead=0;
	FILE *fp; 
	
 
	
	if (argc < 4) 
	{
	       fprintf(stderr,"Usage: %s <hostname> <port> <filename>\n", argv[0]);
	       exit(1);
	}
	
	
	//get host and port
	server = gethostbyname(argv[1]);
	portnum = atoi(argv[2]);
	if(portnum==0) error("Incorrect port Number given. Please give a correct input. ");
	
	
	//parse file name
	strcpy(filename, argv[3]);
	
	fp=fopen(filename, "wb");
	
	//create client socket
	s = socket(AF_INET, SOCK_STREAM, 0);
	if(s<0) error("Error opening socket");
	
	
	//Set Values
	bzero((char*)&sa, sizeof(sa));
	sa.sin_family = AF_INET;
	sa.sin_port = htons(portnum);
	
	
	//copy servers' address into the socket address. 
	bcopy((char*)server->h_addr_list[0], (char*)&sa.sin_addr.s_addr, server->h_length);
	
	
	//Try connecting to the Socket
	if(connect(s, (struct sockaddr*)&sa,sizeof(sa)) <0) error("Error connecting to socket");
	
	
	//printf("Enter the message to send to socket.");
	
	//Clear the buffer
	bzero(buffer, sizeof(buffer));
	
	
	//Create the request for file
	sprintf(buffer, "Get /%s HTTP/1.1 \n\n", filename);
	
	
	//fgets(buffer,sizeof(buffer)-1, stdin);
 
	
	
	//Send request on the socket
	n = write(s,buffer, strlen(buffer));
	if(n<0) error("Cannot write to socket.");
	
	
		//Clear buffer to read data
		bzero(buffer, sizeof(buffer));
		bzero(buffer2, sizeof(buffer2));
		
		//Read data from the socket for the first time
		n=read(s, buffer, sizeof(buffer)-1);
		if(n==-1) error("Socket read error. ");
		
		//get the file size from the header
		fileSize = findFileSize((char*)buffer);
		
		//printf("File Size read = %lu Bytes\n", fileSize);
		printf("The message read from the socket is: %s \n\n", buffer);
		
		//Remove header from the message to write into file 
		strcpy(buffer2, removeHeader((char*)buffer));
		
		printf("Message without the header: %s \n", buffer2);
		
		printf("Size of reduced message= %d", strlen(buffer2));
 
		
		sizeAlreadyRead = strlen(buffer2);
		fwrite(buffer2, sizeof(char), strlen(buffer2), fp);
		
		while(sizeAlreadyRead < fileSize-BUFFER_LENGTH + 1)
		{
			bzero(buffer, sizeof(buffer));
			n=read(s, buffer, sizeof(buffer)-1);
			if(n==-1) error("Socket read error. ");
			
			
			
			sizeAlreadyRead += sizeof(buffer)-1;
			printf("Size already read: %lu\n", sizeAlreadyRead);
			fwrite(buffer, sizeof(char), sizeof(buffer)-1, fp);
			
		}
		
		// Write the remaining bytes - which are less than BUFFER_LENGTH
		printf("Now transfering last amount of data. %lu",fileSize-sizeAlreadyRead);
		bzero(buffer, sizeof(buffer));
		n=read(s, buffer, fileSize-sizeAlreadyRead+1);
		if(n==-1) error("Socket read error. ");
		
		//printf("The message read from the socket is: %s\n", buffer);
		
		//sizeAlreadyRead += sizeof(buffer);
		fwrite(buffer, sizeof(char), fileSize-sizeAlreadyRead+1, fp);		
		
		
	
	fclose(fp);
	printf("File saved successfully!! ");
	if(n<0) error("Cannot write to socket.");
	//printf("The message read from the socket is: %s", buffer);
	
	return 0;
	
}
 
unsigned long findFileSize(char* header)
{
	unsigned long size=0;
	
	//find out the position of the substring "Content-Length: " to know the file size. 
	char* pos = strstr(header, "Content-Length: ");
	//printf("%s", pos);
	sscanf(pos, "%*s %lu", &size);
	//printf("The file size is %lu\n ", size);
	return size; 
	
}
 
char* removeHeader(char *string)
{
	char* res;
	char *pos=strstr(string, "Connection: close");
	
	//19 here is the length of "Connection: close", which needs to be counted on after the location it is found at
	//to get the remaining string. 
	int size = strlen(pos+19);
	res = (char*)malloc(size*sizeof(char));
	strcpy(res, pos+19);
	return res; 
}
 
 
void error(char *msg)
{
	perror(msg);
	exit(1);
}

Open in new window

0
piyush_soni
Asked:
piyush_soni
  • 6
  • 3
  • 2
2 Solutions
 
Infinity08Commented:
1) This :

        sprintf(buffer, "Get /%s HTTP/1.1 \n\n", filename);

    should be :

        sprintf(buffer, "Get /%s HTTP/1.1\r\nHost: %s\r\n\r\n", filename, argv[1]);

    to be conform the HTTP/1.1 standard. The host HAS to be specified, and lines need to be terminated with a CR/LF pair, not just an LF.


2) Instead of write :

        n = write(s,buffer, strlen(buffer));

    I'd use send :

        n = write(s, buffer, strlen(buffer), 0);

    and instead of read :

        n=read(s, buffer, sizeof(buffer)-1);

    I'd use recv :

        n = recv(s, buffer, sizeof(buffer), 0);

    Note that I used the entire buffer ... No sense in appending a null byte, especially if it's binary data. Instead of the NULL byte, you can use the double \r\n at the end of the headers to detect the end of the headers and the start of the data.


3) 256 bytes is a bit small for a receive buffer ... Better make that a bit bigger, like 4096 bytes.


4) your removeHeader function allocates memory which is never freed ... That's a memory leak !! Also, as said in point 2), instead of removeHeader, you'd better use the double \r\n to detect the start of the data.


5) you use strlen to calculate the length of the buffer. You shouldn't do that. Use the value returned by recv to know how many bytes were received, as well as the value from the Content-Length header. In case of binary data for example, it might contain zero bytes, so strlen will NOT see the entire data.



I didn't look much further, because the above are already a few problems that would cause the behavior you see. Fix all of them, and try again. If there are still problems, let me know, and I'll look further ;)
0
 
evilrixSenior Software Engineer (Avast)Commented:
Observations: -

1. (line 174) The malloc'd memory returned by removeHeader() is never free'd so this is a memory leak.
2. (line 95) The return value from read is the amount of data actually read. Apart from checking for error you just assume the amount you've read is what you expected to read.
3. (line 50) No check to see if the file open was successful
4. (line 123) Subtract n and not sizeof(buffer) -1 as you might not have read this much
5. (line 125) The amount you write should be n (the amount you read) and not sizeof(buffer), which may be greater so you'll write rubbish
6. (line 138) Same as point 5 abive

Above is a summary of some obvious issues. I think the root cause of your problem is the assumptions you are making about how much you've actually read vs. how much you write. Although in an ideal world what you request is what you'll get the reality is different. To be robust you need to make proper use of the return value from read. In essence, you are making too many assumptions (such as not checking files have been opened successfully). Can I suggest you look at resolving these issues and then if you still have a problem repost the code with the changes and we can take it from there. Meanwhile I look through the code in more detail and see if I can spot any other issues.

-Rx.
0
 
Infinity08Commented:
>>     I'd use send :
>> 
>>         n = write(s, buffer, strlen(buffer), 0);

should have been :

        n = send(s, buffer, strlen(buffer), 0);

of course lol
0
Keep up with what's happening at Experts Exchange!

Sign up to receive Decoded, a new monthly digest with product updates, feature release info, continuing education opportunities, and more.

 
evilrixSenior Software Engineer (Avast)Commented:
EE could really do with a code review mechanism... like CodeStriker :)
0
 
evilrixSenior Software Engineer (Avast)Commented:
Why is this cast necessary?
I have gone through you code and annotated it with comments where I have spotted obvious issues of possible refactoring opportunities. Obviously, I can't try/test this code so I've just done the best I can by reading through it. Please note: -

(a) some of my assumptions may be wrong, so let's just say I pointed out what looks like something you need to consider
(b) I'm not claiming that the issues I've found are either a complete list or the cause of your problem -- they were just, ostensibly, obvious issues for you to look at
(c) I note that I8 has also spotted other additional things that you should also look at too.

Please feel free to discuss any of the comments/suggestions/observations I've made.

I hope this helps you.

-Rx.
#include<stdio.h>
#include<sys/types.h>
#include<sys/socket.h>
#include<netinet/in.h>
#include<netdb.h>
#include<stdlib.h>
#include<string.h>
#include<unistd.h>
 
#define BUFFER_LENGTH 256 //<--- RX: This isn't much of a buffer, I'd go for at least 1024 bytes
 
 
//Function Declarations:
unsigned long findFileSize(char* header);
char* removeHeader(char *string);
void error(char *msg);
 
 
 
int main(int argc, char*argv[])
{
	int s;
	int portnum;
	struct sockaddr_in sa;
	struct hostent *server;
	int n;
	char buffer[BUFFER_LENGTH], buffer2[BUFFER_LENGTH];
	char filename[100];
	unsigned long fileSize=0, sizeAlreadyRead=0;
	FILE *fp; 
 
 
 
	if (argc < 4) 
	{
		fprintf(stderr,"Usage: %s <hostname> <port> <filename>\n", argv[0]);
		exit(1);
	}
 
 
	//get host and port
	server = gethostbyname(argv[1]); //<--- RX: This can fail (returns NULL), you need to check for this
	portnum = atoi(argv[2]);
	if(portnum==0) error("Incorrect port Number given. Please give a correct input. ");
 
 
	//parse file name
	strcpy(filename, argv[3]); //<--- RX: This is a potential buffer overrrun if argv[3] size > filename size
 
	fp=fopen(filename, "wb"); //<--- RX: (a) did it open (b) have you just overwritten an important file?
 
	//create client socket
	s = socket(AF_INET, SOCK_STREAM, 0);
	if(s<0) error("Error opening socket");
 
 
	//Set Values
	bzero((char*)&sa, sizeof(sa)); //<--- RX: param 1 of bzero is void * so the cast is unnecessary
	sa.sin_family = AF_INET;
	sa.sin_port = htons(portnum);
 
 
	//copy servers' address into the socket address. 
	bcopy((char*)server->h_addr_list[0], (char*)&sa.sin_addr.s_addr, server->h_length); //<--- RX: Unnecessary casts as params 1 and 2 are void *
 
 
	//Try connecting to the Socket
	if(connect(s, (struct sockaddr*)&sa,sizeof(sa)) <0) error("Error connecting to socket");
 
 
	//printf("Enter the message to send to socket.");
 
	//Clear the buffer
	bzero(buffer, sizeof(buffer));
 
 
	//Create the request for file
	sprintf(buffer, "Get /%s HTTP/1.1 \n\n", filename);
 
 
	//fgets(buffer,sizeof(buffer)-1, stdin);
 
 
 
	//Send request on the socket
	n = write(s,buffer, strlen(buffer));
	if(n<0) error("Cannot write to socket.");
 
 
	//Clear buffer to read data
	bzero(buffer, sizeof(buffer));
	bzero(buffer2, sizeof(buffer2));
 
	//Read data from the socket for the first time
	n=read(s, buffer, sizeof(buffer)-1); //<--- RX: Why -1? This is a buffer not a string -- below you use it as a string but it is not so you may end up with buffer overruns
	if(n==-1) error("Socket read error. ");
 
	//get the file size from the header
	fileSize = findFileSize((char*)buffer); //<--- RX: How do you know you've read enough, you never check n to see?
 
	//printf("File Size read = %lu Bytes\n", fileSize);
	printf("The message read from the socket is: %s \n\n", buffer); //<--- RX: Assumes buffer is null terminated; what if it's not?
 
	//Remove header from the message to write into file 
	strcpy(buffer2, removeHeader((char*)buffer)); //<--- RX: Isn't this cast unnecessary as param2 is a char*?
 
	printf("Message without the header: %s \n", buffer2); //<--- RX: Is buffer2 null terminated?
 
	printf("Size of reduced message= %d", strlen(buffer2)); //<--- RX: Is buffer2 null terminated?
 
 
	sizeAlreadyRead = strlen(buffer2); //<--- RX: Is buffer2 null terminated? You already calculated strlen above!
	fwrite(buffer2, sizeof(char), strlen(buffer2), fp); //<--- RX: Callin strlen again is unnecessarily cost, you have sizeAlreadyRead available
 
	while(sizeAlreadyRead < fileSize-BUFFER_LENGTH + 1) //<--- RX: Im a little unsure why you need -1/+1 with these buffers
	{
		bzero(buffer, sizeof(buffer));
		n=read(s, buffer, sizeof(buffer)-1);
		if(n==-1) error("Socket read error. ");
 
 
 
		sizeAlreadyRead += sizeof(buffer)-1; //<--- RX: += n would be safer since n may not be sizeof(buffer)
		printf("Size already read: %lu\n", sizeAlreadyRead);
		fwrite(buffer, sizeof(char), sizeof(buffer)-1, fp); //<--- RX: write n and not sizeof(buffer) -1 since the two may not be the same
 
	}
 
	// Write the remaining bytes - which are less than BUFFER_LENGTH
	printf("Now transfering last amount of data. %lu",fileSize-sizeAlreadyRead);
	bzero(buffer, sizeof(buffer));
	n=read(s, buffer, fileSize-sizeAlreadyRead+1);
	if(n==-1) error("Socket read error. ");
 
	//printf("The message read from the socket is: %s\n", buffer);
 
	//sizeAlreadyRead += sizeof(buffer);
	fwrite(buffer, sizeof(char), fileSize-sizeAlreadyRead+1, fp); //<--- RX: Again you assume n and amount read are the same, they may not be. You should have dome all this int he loop above until there was no more left to read!
 
 
 
	fclose(fp);
	printf("File saved successfully!! ");
	if(n<0) error("Cannot write to socket."); //<--- RX: What's this doing here?
	//printf("The message read from the socket is: %s", buffer);
 
	return 0;
 
}
 
unsigned long findFileSize(char* header)
{
	unsigned long size=0;
 
	//find out the position of the substring "Content-Length: " to know the file size. 
	char* pos = strstr(header, "Content-Length: "); //<--- RX: THis can return null, you need to test for this
	//printf("%s", pos);
	sscanf(pos, "%*s %lu", &size); //<--- RX: What if pos is null?
	//printf("The file size is %lu\n ", size);
	return size;  //<--- RX: What fi you didn't find size?
 
}
 
char* removeHeader(char *string)
{
	char* res;
	char *pos=strstr(string, "Connection: close"); //<--- RX: This can return null
 
	//19 here is the length of "Connection: close", which needs to be counted on after the location it is found at
	//to get the remaining string. 
	int size = strlen(pos+19); //<--- RX: This will crash if pos is null
	res = (char*)malloc(size*sizeof(char)); //<--- RX: This leaks!
	strcpy(res, pos+19); //<--- RX: You assume pos+19 is null terminated -- is it?
	return res;  //<--- RX: Returns memory that leaks!
}
 
 
void error(char *msg)
{
	perror(msg);
	exit(1);
}

Open in new window

0
 
evilrixSenior Software Engineer (Avast)Commented:
>> Why is this cast necessary?
Ignore that above... it was an accidental copy/paste fopar :)
0
 
piyush_soniAuthor Commented:
Thank you friends, I was finally able to solve the problem with your tips. :)
0
 
piyush_soniAuthor Commented:
Evilrix, I wanted to accept your solution as "Assisted Solution" but mistakingly I forgot to click 'Accept multiple solutions" it seems.... There is no way to revert back and accept your one as the supporting solution.  Any idea to do the same? :)  Thanks for your efforts !
0
 
evilrixSenior Software Engineer (Avast)Commented:
Hi piyush_soni,

I have requested the Q be reopened so you can close it as you intended.

Thanks.

-Rx.
0
 
evilrixSenior Software Engineer (Avast)Commented:
Hi piyush_soni,

This has been reopened (thanks VM) so you can close it as you originally intended.

Thanks.

-Rx.
0
 
piyush_soniAuthor Commented:
Though the comments given by Infinity08 were sufficient to solve the problem and they were given earlier, but evilrix also suggested some very good points and did a lot of work.
0

Featured Post

Free Tool: Path Explorer

An intuitive utility to help find the CSS path to UI elements on a webpage. These paths are used frequently in a variety of front-end development and QA automation tasks.

One of a set of tools we're offering as a way of saying thank you for being a part of the community.

  • 6
  • 3
  • 2
Tackle projects and never again get stuck behind a technical roadblock.
Join Now