2

I have code where I send file from server to client. The thing is client correctly receives file size from the server, the file (jpeg image), is also recreated with correct file size on the client side, but when I open the image it is displayed half correctly, half is corrupted.

Here is some relevant parts of code. Any idea why this can be happening?

server:

  while(1)
  {
        printf("Waiting to accept connections \n");
        connfd = accept(listenfd, (struct sockaddr*)NULL, NULL);

        read_file("/Users/macbookair/Documents/server/server/file1.jpeg", buffer, &length);

        // send
        write(connfd, &length, 4); // length, 110193 bytes
        write(connfd, buffer, length); // file itself

        close(connfd);
        sleep(1);
  }

client:

    // open file
    FILE * ptrMyFile = fopen("/Users/macbookair/Documents/client/client/file1.jpeg","wb");
    if(NULL == ptrMyFile)
    {
        printf("Unable to open file \n");
        return 1;
    }

    int size = 0;
    read(sockfd, &size, 4); // read length first
    read(sockfd, buffer, size); // then file

    printf("fsize %d\n", size);

    // create file
    fwrite(buffer, size, 1, ptrMyFile);
    fclose(ptrMyFile);

buffer is of sufficient length. I find weird that correct number of bytes is received, but image is half correctly displayed.

  • Does `read_file` open the original file with `"rb"`? – aschepler Jun 23 '14 at 18:08
  • @aschepler: yes it does, should it not be doing this? –  Jun 23 '14 at 18:09
  • Yes, that's correct. Next thing I would check: do all the `read` and `write` etc. calls actually process all the data you asked them to in one try? Always check the return code. – aschepler Jun 23 '14 at 18:12
  • @aschepler: yes, that is true I could check that, but on the client side, the file is recreated with correct size (=110193 bytes), so is it still necessary to check that? –  Jun 23 '14 at 18:14
  • 2
    All that proves is that the first four bytes transmitted correctly. If the client's `read(sockfd, buffer, size)` gets less than `size` bytes, that just means some of the bytes written out by `fwrite(buffer, size, 1, ptrMyFile)` are still uninitialized. – aschepler Jun 23 '14 at 18:17
  • @aschepler: so you mean file size is correctly set, but data has not been written completely? –  Jun 23 '14 at 18:18
  • You should not get file size in the manner you have, see: [Using fseek and ftell to determine the size of a file has a vulnerability?](http://stackoverflow.com/questions/5957845/using-fseek-and-ftell-to-determine-the-size-of-a-file-has-a-vulnerability) – David C. Rankin Jun 23 '14 at 18:30
  • @aschepler: how come file had correct size :)) –  Jun 23 '14 at 18:36
  • Because from the first four bytes, you correctly set the client's variable `size`. And then you copied that many bytes from your buffer to the created file, ignoring how many bytes in that buffer actually came from the server. – aschepler Jun 23 '14 at 18:38

1 Answers1

3

Network programming is not like programming with local files.

read(sockfd, &size, 4); // read length first
read(sockfd, buffer, size); // then file

You are ignoring the return value of read() -- which, most critically, tells you how many bytes were actually read. Since this is a network operation, you might get fewer bytes than you asked for. This means that only the first two bytes of size might be read, and then you have to ask again for the next two bytes. This is unlikely, but possible. What is most likely is that the file will be split into chunks if it is larger than a few K.

int read_full(int sock, size_t sz, void *buffer)
{
    size_t pos = 0;
    while (pos < sz) {
        size_t request_amt = sz - pos;
        ssize_t result = read(sock, (char *) buffer + pos, request_amt);
        if (result < 0) {
            // If EINTR, then the read() was interrupted
            // In this case, you have to try again
            // otherwise, an error occurred
            if (errno != EINTR)
                return -1;
        } else if (result > 0) {
            pos += result;
        } else {
            // unexpected EOF
            return -1;
        }
    }
    return 0;
}

In practice, if you send a large amount of data over a TCP connection, it will get split up into chunks and transmitted over a period of time. You have very little control over how big those chunks are.

Here is the corrected code, with assert() for lazy error checking:

uint32_t size;
int r;
r = read_full(sockfd, sizeof(size), &size);
assert(r == 0);
void *data = malloc(size);
assert(data != NULL);
r = read_full(sockfd, size, data);
assert(r == 0);

Important note: write() / send() also can do partial writes, so you will need to write the same loop on the server!

Dietrich Epp
  • 205,541
  • 37
  • 345
  • 415
  • OK first I have to say why there was the confusion. The problem like I said was that the file was created on the client side, with correct size! that is why I got confused that read was getting all bytes (as correctly hinted by aschepler). Now it works, by using readall method similar to this one: beej.us/guide/bgnet/output/html/multipage/advanced.html#sendall. Thanks all for help –  Jun 23 '14 at 18:35
  • It would have the correct size... but that's because the program always wrote the correct number of bytes, even if not all of those bytes had been read from the server. – Dietrich Epp Jun 23 '14 at 18:38