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

#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);
}