Server not able to properly read/open a filename sent by client in C

Question

Hi, I'm doing client/server interaction with sockets in C. What I'm trying to do is have the client request a file to be read on the server, the server return a buffer of the file contents, and the client print out the file.

Though I was able to accomplish the server sending a buffer of a file to the client & the client printing it out successfully, I can't seem to get the server to successfully read a filename sent by the client.

Here is what I mean:

///This is the structure of the message that gets sent back and forth
struct message { 
int code; //Just indicates 1 or 2 for readfile or writefile
int size;
char buffer[256]; //This will hold the filename when client sends readfile request
 };

Works:

char *filename = "test.c";
infile = open(filename, O_RDONLY);
//Send the file and everything back to the client

Doesn't Work:

while( read(sockfd, &msg, sizeof(int) * 2) > 0) {
        if(msg.code == 1) { //Perform a read operation
            int infile, filesize, rr;
            int length;
            char output[256];
            size_t rb = 0;

            while(rb < msg.size) { 
                ssize_t r = read(sockfd, msg.buffer, msg.size - rb);
                if(r < 0) { 
                    error(sockfd, r, msg.buffer);
                    break;
                }
                rb += r;
            }

            msg.buffer[rb] = '\0';  
         //This has been printing out the correct amount
            printf("\nBytes read: %i", rb); 
         //This has also been printing out properly
            printf("\nmsg.buffer: %s", msg.buffer); 

            infile = open(msg.buffer, O_RDONLY);

I've edited to show the current state my program is in, and is still not working. Before, I had a strcpy that was improperly placed.

Answer 1

You are not checking return value of read(2), which tells how many bytes have been read, - your message might not be read in full, so the file name can be truncated.

Edit:

@Amardeep here has better eyes them me - you are corrupting your memory with strcpy(3) into space pointed to by filename pointer variable, which looks like uninitialized.

Answer 2

In the code you posted, filename is not initialized. Your compiler should have warned you about this. If it didn't, you're not invoking it correctly; with gcc, do at least gcc -O -Wall.

You also need to allocate memory for the file name if you use strcpy. This step is not necessary in a simple program; making a copy becomes useful if you need to remember the file name after you go on reading from the client. The strdup function combines memory allocation with string copying, it's appropriate here.

You need to check the return value of all system calls. The return value of read tells you how many bytes were read. The call to read(fd,buf,n) is allowed to return less than n bytes if the OS feels like it. If you received fewer bytes than you expected, call read in a loop. Yes, pretty much all programs that call read call it in a loop, it's a basic Unix/POSIX idiom. The fread function does it for you, if you can fit it in.

The code to check the validity of msg.code and msg.size is missing. Since you've allocated 256 bytes in msg.buffer, you must limit msg.size to 255.

Yes, the assignment msg.buffer[msg.size] = '\0' is necessary, because open needs the '\0' character at the end of the name (that's how it knows where the name ends).

---

I thought maybe strcpy was appropriate

Whenever you're around pointers (which is most of the time in C), you need to think carefully about what you're doing and where those pointers are pointing, and whether there's enough room there for what you want to put. C is an unforgiving language; throwing pointers around is as dangerous as throwing arrows around. Draw diagrams! There are two kinds of C programmers: the ones who draw diagrams on a whiteboard, on paper, in the sand or other medium; and the ones who draw diagrams in their head (the third kind are still trying to figure out why their program to print 1+1 is printing 3).

Answer 3

Well I've about found out the answer, or at least something close to it. First off, a line break character was getting sent. On the client side, I used fgets(msg.buffer, 256, stdin); to get the message. When I checked the value using printf("File |%s|", msg.buffer), I saw that the second bar was on the next line. I took that line break and overwrote it with a null character at the end.

I also changed to fopen and freads/writes. I added ./ to the beginning of the file statement but I doubt that's needed.. it was just one of my earlier attempts. It now looks something like this:

            size_t rb = 0;
            FILE* file;
            char filename[256];
            while(rb < msg.size) { 
                ssize_t r = read(sockfd, filename, msg.size - rb);
                if(r < 0) { 
                    error(sockfd, r, msg.buffer);
                    goto end;
                }
                rb += r;
            }
            if(strlen(filename) > 253) { 
                error(sockfd, rb, msg.buffer);
                goto end;
            }
            strcpy(msg.buffer, "./");
            strcat(msg.buffer, filename);
            msg.buffer[rb + 1] = '\0';

            file = fopen(msg.buffer, "r");
            if(file == NULL) {
                error(sockfd, rb, msg.buffer);
                printf("Error opening file %s: %s\n", msg.buffer,strerror(errno));
                fflush(stdout);
                goto end;
            }

Thanks to all who helped. I learned a lot (gdb was especially useful -- I never used it before)