1

This is for a Linux system, in C. It involves network programming. It is for a file transfer program.

I've been having this problem where this piece of code works unpredictably. It either is completely successful, or the while loop in the client never ends. I discovered that this is because the fileLength variable would sometimes be a huge (negative or positive) value, which I thought was attributed to making some mistake with ntohl. When I put in a print statement, it seemed to work perfectly, without error.

Here is the client code:

        //...here includes relevant header files

        int main (int argc, char *argv[]) {
            //socket file descriptor
            int sockfd;

            if (argc != 2) {
                fprintf (stderr, "usage: client hostname\n");
                exit(1);
            }

            //...creates socket file descriptor, connects to server


            //create buffer for filename
            char name[256];
            //recieve filename into name buffer, bytes recieved stored in numbytes
            if((numbytes = recv (sockfd, name, 255 * sizeof (char), 0)) == -1) {
                perror ("recv");
                exit(1);
            }
            //Null terminator after the filename
            name[numbytes] = '\0';
            //length of the file to recieve from server
            long fl;
            memset(&fl, 0, sizeof fl);
            //recieve filelength from server
            if((numbytes = recv (sockfd, &fl, sizeof(long), 0)) == -1) {
                perror ("recv");
                exit(1);
            }

            //convert filelength to host format
            long fileLength = ntohl(fl);


            //check to make sure file does not exist, so that the application will not overwrite exisitng files
            if (fopen (name, "r") != NULL) {
                fprintf (stderr, "file already present in client directory\n");
                exit(1);
            }
            //open file called name in write mode
            FILE *filefd = fopen (name, "wb");
            //variable stating amount of data recieved
            long bytesTransferred = 0;
            //Until the file is recieved, keep recieving
            while (bytesTransferred < fileLength) {
                printf("transferred: %d\ntotal: %d\n", bytesTransferred, fileLength);
                //set counter at beginning of unwritten segment
                fseek(filefd, bytesTransferred, SEEK_SET);
                //buffer of 256 bytes; 1 byte for byte-length of segment, 255 bytes of data
                char buf[256];
                //recieve segment from server
                if ((numbytes = recv (sockfd, buf, sizeof buf, 0)) == -1) {
                    perror ("recv");
                    exit(1);
                }

                //first byte of buffer, stating number of bytes of data in recieved segment
                //converting from char to short requires adding 128, since the char ranges from -128 to 127
                short bufLength = buf[0] + 128;

                //write buffer into file, starting after the first byte of the buffer
                fwrite (buf + 1, 1, bufLength * sizeof (char), filefd);
                //add number of bytes of data recieved to bytesTransferred
                bytesTransferred += bufLength;

            }
            fclose (filefd);
            close (sockfd);

            return 0;
        }

This is the server code:

        //...here includes relevant header files

        int main (int argc, char *argv[]) {
            if (argc != 2) {
                fprintf (stderr, "usage: server filename\n");
                exit(1);
            }
            //socket file descriptor, file descriptor for specific client connections
            int sockfd, new_fd;


            //...get socket file descriptor for sockfd, bind sockfd to predetermined port, listen for incoming connections



            //...reaps zombie processes


            printf("awaiting connections...\n");

            while(1) {
                //...accepts any incoming connections, gets file descriptor and assigns to new_fd

                if (!fork()) {
                    //close socket file discriptor, only need file descriptor for specific client connection
                    close (sockfd);
                    //open a file for reading
                    FILE *filefd = fopen (argv[1], "rb");
                    //send filename to client
                    if (send (new_fd, argv[1], strlen (argv[1]) * sizeof(char), 0) == -1)
                    { perror ("send"); }
                    //put counter at end of selected file, and  find length
                    fseek (filefd, 0, SEEK_END);
                    long fileLength = ftell (filefd);
                    //convert length to network form and send it to client

                    long fl = htonl(fileLength);
                    //Are we sure this is sending all the bytes??? TEST
                    if (send (new_fd, &fl, sizeof fl, 0) == -1)
                    { perror ("send"); }
                    //variable stating amount of data unsent
                    long len = fileLength;
                    //Until file is sent, keep sending
                    while(len > 0) {
                        printf("remaining: %d\ntotal: %d\n", len, fileLength);
                        //set counter at beginning of unread segment
                        fseek (filefd, fileLength - len, SEEK_SET);
                        //length of the segment; 255 unless last segment
                        short bufLength;
                        if (len > 255) {
                            len -= 255;
                            bufLength = 255;
                        } else {
                            bufLength = len;
                            len = 0;
                        }
                        //buffer of 256 bytes; 1 byte for byte-length of segment, 255 bytes of data
                        char buf[256];
                        //Set first byte of buffer as the length of the segment
                        //converting short to char requires subtracting 128
                        buf[0] = bufLength - 128;
                        //read file into the buffer starting after the first byte of the buffer
                        fread(buf + 1, 1, bufLength * sizeof(char), filefd);
                        //Send data too client
                        if (send (new_fd, buf, sizeof buf, 0) == -1)
                        { perror ("send"); }
                    }
                    fclose (filefd);
                    close (new_fd);
                    exit (0);
                }
                close (new_fd);
            }

            return 0;
        }

Note: I've simplified the code a bit, to make it clearer I hope. Anything beginning with //... represents a bunch of code

Joshua Perrett
  • 354
  • 4
  • 10
  • 4
    I don't see any synchronization mechanism. The peers exchange a raw binary data without any indication of what this data is. So part of your `name` can be easily interpreted as length. You need to define a *protocol*. – Eugene Sh. Sep 27 '16 at 13:35
  • The type `numbytes, sin_size` are unknown as not declared in post. AKAIK, they have a type that is causing the issue. Better to post their declarations. Example `socklen_t sin_size = sizeof their_addr;` – chux - Reinstate Monica Sep 27 '16 at 13:39
  • How would this cause problems though, in this case? (Sorry, I'm new to this.) I told it to send and receive data in the right order in correct sizes, no? – Joshua Perrett Sep 27 '16 at 13:40
  • `recv` is receiving *as much data as currently available*. Think what happens if only a part of the first message was available when `recv` was called. – Eugene Sh. Sep 27 '16 at 13:44
  • With post change, what is the value of `new_fd` when `send (new_fd, argv[1], strlen (argv[1]) * sizeof(char), 0)` is called? It appears uninitialized. – chux - Reinstate Monica Sep 27 '16 at 13:46
  • @chux there's a lot of code not shown, and I've replaced it with comments summarizing it, because I know it to work. Although, would it be good to post all of it anyway? :/ – Joshua Perrett Sep 27 '16 at 13:47
  • @chux new_fd is the file descriptor for the specific client connection. It is initialized in my code here, but I've summarized it – Joshua Perrett Sep 27 '16 at 13:49
  • @Eugene Sh. That makes sense. So how would I do as you've suggested? Brief example, explanation? – Joshua Perrett Sep 27 '16 at 13:52
  • At the very least define the field lengths and *loop* on the `recv` until got the whole length of the field expected. If there is a "leftover" - append it to the next field. Or there are some other techniques you should research. – Eugene Sh. Sep 27 '16 at 13:54
  • Define the field lengths? But I'm not sure of the name length. I could always overestimate, but that seems clunky. Should I then have the first byte of the name be the number of bytes of the name? would that work? – Joshua Perrett Sep 27 '16 at 14:00
  • The only "non-4"-case in `recv`ing `fl` you catch is -1. Note `recv` could very well also return 1, 2, or 3, up to 8 on a 64-bit system - In this case, your code would need to loop around until you get the `sizeof unsigned long` bytes altogether. – tofro Sep 27 '16 at 14:00
  • @JoshuaPerrett There are two approaches - either fixed-length fields or unique delimiters. Choose one. You can combine them, though.. Anyway, this topic is too broad for comments (and even answers..). – Eugene Sh. Sep 27 '16 at 14:02
  • @EugeneSh. Thanks. – Joshua Perrett Sep 27 '16 at 14:13
  • @tofro Yes, good idea. But I thought a long only had 4 bytes, why would it go up to 8, if recv returns the number of bytes received? And another dumb question, "sizeof unsigned long" should be the same as "sizeof long" right? – Joshua Perrett Sep 27 '16 at 14:16
  • @JoshuaPerrett `sizeof long`depends on your platform, typically 4 bytes on a 32-bit, and 8 bytes on a 64-bit architecture - You need to find out yourself. And `sizeof unsigned long` is guaranteed to be the same size. – tofro Sep 27 '16 at 14:18
  • @torfro correct me if i'm wrong then, I should use int32_t if I want to specify that I want a 4 byte integer type, regardless of architecture? Couldn't that be another issue? I'm using a 64bit OS, and ntohl() is for 4 byte integer types, right? – Joshua Perrett Sep 27 '16 at 14:31
  • @torfro sorry to ask again (I have tried looking it up), but are you saying the size of an unsigned long is same on 32 bit and 64 bit operating systems? Why would that be the case? – Joshua Perrett Sep 27 '16 at 14:37
  • No, I'm not saying that - where do you read this? I'm just saying that a `signed long` and an `unsigned long` are the same size – tofro Sep 27 '16 at 16:58
  • @tofro Ok, that makes sense. I was just misunderstanding what you wrote. Thanks for your help. – Joshua Perrett Sep 28 '16 at 07:22

4 Answers4

3

You seem to be assuming that each send() will either transfer the full number of bytes specified or will error out, and that each one will will pair perfectly with a recv() on the other side, such that the recv() receives exactly the number of bytes sent by the send() (or error out), no more and no less. Those are not safe assumptions.

You don't show the code by which you set up the network connection. If you're using a datagram-based protocol (i.e. UDP) then you're more likely to get the send/receive boundary matching you expect, but you need to account for the possibility that packets will be lost or corrupted. If you're using a stream-based protocol (i.e. TCP) then you don't have to be too concerned with data loss or corruption, but you have no reason at all to expect boundary-matching behavior.

You need at least three things:

  • An application-level protocol on top of the network-layer. You've got parts of that already, such as in how you transfer the file length first to advise the client about much content to expect, but you need to do similar for all data transferred that are not of pre-determined, fixed length. Alternatively, invent another means to communicate data boundaries.

  • Every send() / write() that aims to transfer more than one byte must be performed in a loop to accommodate transfers being broken into multiple pieces. The return value tells you how many of the requested bytes were transferred (or at least how many were handed off to the network stack), and if that's fewer than requested you must loop back to try to transfer the rest.

  • Every recv() / read() that aims to transfer more than one byte must be performed in a loop to accommodate transfers being broken into multiple pieces. I recommend structuring that along the same lines as described for send(), but you also have the option of receiving data until you see a pre-arranged delimiter. The delimiter-based approach is more complicated, however, because it requires additional buffering on the receiving side.

Without those measures, your server and client can easily get out of sync. Among the possible results of that are that the client interprets part of the file name or part of the file content as the file length.

John Bollinger
  • 160,171
  • 8
  • 81
  • 157
1

Even though you removed it from that code I'll make an educated guess and assume that you're using TCP or some other stream protocol here. This means that the data that the servers sends is a stream of bytes and the recv calls will not correspond in the amount of data they get with the send calls.

It is equally legal for your first recv call to just get one byte of data, as it is to get the file name, file size and half of the file.

You say

When I put in a print statement,

but you don't say where. I'll make another educated guess here and guess that you did it on the server before sending the file length. And that happened to shake things enough that the data amounts that were sent on the connection just accidentally happened to match what you were expecting on the client.

You need to define a protocol. Maybe start with a length of the filename, then the filename, then the length of the file. Or always send 256 bytes for the filename regardless of how long it is. Or send the file name as a 0-terminated string and try to figure out the data from that. But you can never assume that just because you called send with X bytes that the recv call will get X bytes.

Art
  • 19,807
  • 1
  • 34
  • 60
  • Yes, you were right, it was TCP. Could you elaborate/simplify by what you mean when you said using a stream protocol "means that the data that the servers sends is a stream of bytes and the recv calls will not correspond in the amount of data they get with the send calls." Also, I had the prinf statements just after receiving the file and just after putting that through ntohl(). But it shouldn't change what you said, I assume. – Joshua Perrett Sep 27 '16 at 14:11
1

I believe the issue is actually a compound of everything you and others have said. In the server code you send the name of the file like this:

send (new_fd, argv[1], strlen (argv[1]) * sizeof(char), 0);

and receive it in the client like this:

recv (sockfd, name, 255 * sizeof (char), 0);

This will cause an issue when the filename length is anything less than 255. Since TCP is a stream protocol (as mentioned by @Art), there are no real boundaries between the sends and recvs, which can cause you to receive data in odd places where you are not expecting them.

My recommendation would be to first send the length of the filename, eg:

// server
long namelen = htonl(strlen(argv[1]));
send (new_fd, &namelen, 4, 0);
send (new_fd, argv[1], strlen (argv[1]) * sizeof(char), 0);
// client
long namelen;
recv (sockfd, &namelen, 4, 0);
namelen = ntohl(namelen);
recv (sockfd, name, namelen * sizeof (char), 0);

This will ensure that you are always aware of exactly how long your filename is and makes sure that you aren't accidentally reading your file length from somewhere in the middle of your file (which is what I expect is happening currently).

edit.

Also, be cautious when you are sending sized numbers. If you use the sizeof call on them, you may be sending and receiving different sizes. This is why I hard-coded the sizes in the send and recv for the name length so that there is no confusion on either side.

Joel C
  • 2,958
  • 2
  • 15
  • 18
  • I think you might be right, I probably misinterpreted what I saw (might not have been stuck in the while loop). Maybe the first recv on the client side (for the file name) got the bytes for both the file name and the file length sometimes. So sometimes I ran it and it would be fine, and other times it would block on the second recv on the client side. – Joshua Perrett Jul 16 '21 at 18:28
0

Well, after some testing, I discovered that the issue causing the problem did have something to do with htonl(), though I had still read the data incorrectly in the beginning. It wasn't that htonl() wasn't working at all, but that I didn't realize a 'long' has different lengths depending on system architecture (thanks @tofro). That is to say the length of a 'long' integer on 32-bit and 64-bit operating systems is 4 bytes and 8 bytes, respectively. And the htonl() function (from arpa/inet.h) for 4-byte integers. I was using a 64-bit OS, which explains why the value was being fudged. I fixed the issue by using the int32_t variable (from stdint.h) to store the file length. So the main issue in this case was not that it was becoming out of sync (I think). But as for everyone's advice towards developing an actual protocol, I think I know what exactly you mean, I definitely understand why it's important, and I'm currently working towards it. Thank you all for all your help.

EDIT: Well now that it has been several years, and I know a little more, I know that this explanation doesn't make sense. All that would result from long being larger than I expected (8 bytes rather than 4) is that there's some implicit casting going on. I used sizeof(long) in the original code rather than hardcoding it to assume 4 bytes, so that particular (faulty) assumption of mine shouldn't have produced the bug I saw.

The problem is almost certainly what everyone else said: one call to recv was not getting all of the bytes representing the file length. At the time I doubted this was the real cause of the behaviour I saw, because the file name (of arbitrary length) I was sending through was never partially sent (i.e. the client always created a file of the correct filename). Only the file length was messed up. My hypothesis at the time was that recv mostly respected message boundaries, and while recv can possibly only send part of the data, it was more likely that it was sending it all and there was another bug in my code. I now know this isn't true at all, and TCP doesn't care.

I'm a little curious as to why I didn't see other unexpected behaviour as well (e.g. the file name being wrong on the receiving end), and I wanted to investigate further, but despite managing to find the files, I can't seem to reproduce the problem now. I suppose I'll never know, but at least I understand the main issue here.

Joshua Perrett
  • 354
  • 4
  • 10
  • Indeed the size of a `long` -- and of all the integer types -- may vary from system to system. C establishes minimum logical sizes for these, not exact sizes. Be aware, however, that there is no rule associating the sizes of these types with system architecture. C implementations on 32-bit machines *can* provide 64-bit `int`s, and implementations on 64-bit machines *can* provide 32-bit `long`s. – John Bollinger Oct 12 '16 at 15:15