Transfer file in C delievers mangled data

Question

I'm a newbie in C programming. I am writing a function where the client copies a file from the server. However, when I open my newly created file it contains a lot of additional characters. How can I prevent it from copying useless data?

Relevant sections of the server follow

            if (file = fopen(buf, "r")){                
            //send the file     
//              while(fgets(buffer, 1024, file) != NULL){
//                  res = write(new_fd, &buffer, sizeof(buffer));
//              }

            while(!feof(file)){
                fscanf(file,"%s",buffer);
                write(new_fd, &buffer, sizeof(buffer));
            }               
            fclose(file);               
        }

Relevant sections of the client follow

        fp = fopen ("testfile", "w");

        while(read(sockfd, &buffer, sizeof(buffer)) != -1){
            fputs(buffer, fp);
        }
        fclose(fp);

Answer 1

read() does not null terminate it's buffer. fputs requires a null terminated buffer.

    while(read(sockfd, &buffer, sizeof(buffer)) != -1){
        fputs(buffer, fp);

You can add the terminator with something like this:

    int n = 0;
    char buffer[SOME_CONSTANT];
    while((n = read(sockfd, buffer, sizeof(buffer - 1))) != -1){
        buffer[n] = 0;
        fputs(buffer, fp);
    }

Note also the declaration and use of buffer vs &buffer in the call to read.

Finally, see all of Joachim Pileborg's suggestions!

Answer 2

Two things: First don't do while (!feof(...)), it doesn't work as you expect it to. The reason is that the EOF flag is not set until after a failed read operation, so you will call fscanf once when the file has already reached the end. Instead to while (fscanf(...) == 1).

Secondly, depending on how you declare buffer, don't use &buffer or sizeof(buffer). Neither in the sender or the receiver. If buffer is a pointer then &buffer will return a pointer to that pointer, and sizeof(buffer) will return the size of the pointer and not what it points to. Besides, if buffer is an array then it might not be completely filled by the input, so why send data you don't need? Only send strlen(buffer) + 1 bytes (the +1 is for the string terminator).

Oh and a third thing, don't use fscanf to read a line, use fgets instead. Or even better, to be more effective, use fread to fill the complete buffer and send it all in less calls.