Why is this C function crashing at fclose?

Question

Hi guys!

Please help me understand why this function throws an exception when it reaches fclose :

void receive_file(int socket, char *save_to, int file_size) {
    FILE *handle = fopen(save_to,"wb");
    if(handle != NULL) {
     int SIZE = 1024;
     char buffer[SIZE];
     memset(buffer,0,SIZE);
     int read_so_far = 0;
     int read_now = 0;
     int reading_unit = (file_size < SIZE) ? file_size : SIZE;
     do {
      read_now = read(socket,buffer,reading_unit);
      fwrite(buffer,read_now,1,handle);
      read_so_far += read_now;
      if(read_so_far >= file_size) {
       break;
      }
      memset(buffer, 0, sizeof(buffer));
     } while (1);
     read_now = 0;
     fclose(handle);
    }
    else {
     extern int errno;
     printf("error creating file");
     printf(" error code : %d",errno);
     exit(-1);
    }
}

Eclipse CDT exists with the following error :

Single stepping until exit from function __kernel_vsyscall, 
which has no line number information.

The purpose of this function is to receive a file through a socket. EDIT: I'm using CentOS 5.3. The thing is, the file is created and written. Even the MD5 is correct. I don't understand why it's failing at fclose. EDIT2: here is a stack trace I managed to get :

*** glibc detected *** /home/linuser/workspace/proj/Debug/proj: free(): invalid next size (normal): 0x096a0068 ***
======= Backtrace: =========
/lib/libc.so.6[0x4fb0f1]
/lib/libc.so.6(cfree+0x90)[0x4febc0]
/lib/libc.so.6(fclose+0x136)[0x4e9c56]
/home/linuser/workspace/proj/Debug/proj[0x8048cd8]
/home/linuser/workspace/proj/Debug/proj[0x80492d6]
/home/linuser/workspace/proj/Debug/proj[0x804963d]
/lib/libc.so.6(__libc_start_main+0xdc)[0x4a7e8c]
/home/linuser/workspace/proj/Debug/proj[0x8048901]
======= Memory map: ========
001ff000-00200000 r-xp 001ff000 00:00 0          [vdso]
0046f000-00489000 r-xp 00000000 08:06 1280361    /lib/ld-2.5.so
00489000-0048a000 r-xp 00019000 08:06 1280361    /lib/ld-2.5.so
0048a000-0048b000 rwxp 0001a000 08:06 1280361    /lib/ld-2.5.so
00492000-005d0000 r-xp 00000000 08:06 1280362    /lib/libc-2.5.so
005d0000-005d2000 r-xp 0013e000 08:06 1280362    /lib/libc-2.5.so
005d2000-005d3000 rwxp 00140000 08:06 1280362    /lib/libc-2.5.so
005d3000-005d6000 rwxp 005d3000 00:00 0 
005d8000-005fd000 r-xp 00000000 08:06 1280369    /lib/libm-2.5.so
005fd000-005fe000 r-xp 00024000 08:06 1280369    /lib/libm-2.5.so
005fe000-005ff000 rwxp 00025000 08:06 1280369    /lib/libm-2.5.so
009b2000-009bd000 r-xp 00000000 08:06 1280372    /lib/libgcc_s-4.1.2-20080825.so.1
009bd000-009be000 rwxp 0000a000 08:06 1280372    /lib/libgcc_s-4.1.2-20080825.so.1
009c5000-00aa5000 r-xp 00000000 08:06 5465873    /usr/lib/libstdc++.so.6.0.8
00aa5000-00aa9000 r-xp 000df000 08:06 5465873    /usr/lib/libstdc++.so.6.0.8
00aa9000-00aaa000 rwxp 000e3000 08:06 5465873    /usr/lib/libstdc++.so.6.0.8
00aaa000-00ab0000 rwxp 00aaa000 00:00 0 
08048000-0804a000 r-xp 00000000 08:06 4884214    /home/linuser/workspace/proj/Debug/proj
0804a000-0804b000 rw-p 00001000 08:06 4884214    /home/linuser/workspace/proj/Debug/proj
096a0000-096c1000 rw-p 096a0000 00:00 0          [heap]
b7e00000-b7e21000 rw-p b7e00000 00:00 0 
b7e21000-b7f00000 ---p b7e21000 00:00 0 
b7f99000-b7f9a000 rw-p b7f99000 00:00 0 
b7faa000-b7fac000 rw-p b7faa000 00:00 0 
bfa82000-bfa97000 rw-p bffea000 00:00 0          [stack]

and here is the "updated" version of the function :

void rec_file(int socket,char *path,int size)
{
    FILE *handle = fopen(path,"wb");
    char buffer[4096];
    int total_read = 0;
    if(handle != NULL)
    {
     while(1)
     {
      int bytes_read = recv(socket,buffer,4096,0);
      total_read    += bytes_read;
      if(bytes_read != -1)
      {
       fwrite(buffer,bytes_read,1,handle);
      }
      else
      {
       printf("read error ");
       exit(-1);
      }
      if(total_read >= size)
      {
       break;
      }
     }
     fclose(handle);
    }
    else
    {
     printf("error receiving file");
     exit(-1);
    }
}

maybe this is cleaner? However, I still receive the same fclose exception.

EDIT3: I commented out everything and I only left the following code, which sadly, still throws the exception at fclose :

void nrec(int sock,char* path,int size)
{
    FILE *handle = fopen(path,"wb");
    if(handle == NULL)
    {
     printf ("error opening file");
     return;
    }
    fclose(handle);
}

Answer 1

This code seems very ... uncertain about itself. Lot of needless assignments, memset() calls, complicated logic.

There's no point in limiting the amount you try to read. Always read as much as you have buffer space for; if there's less data available, you will get less.

It also doesn't properly handle errors. If read() fails, it can return -1, in which case Bad Things(TM) will happen all over the place.

My advice would be to clean it up, think about return values from I/O functions and check them better, then try again.

Failing that, if you're in Linux, run it through Valgrind to get help tracking down the crash.

Answer 2

Not sure specifically, but you do know that read() can return a negative on error, right?

Answer 3

You didn't check whether you got an error indication from the read() call. If you get a -1 from read(), you then pass it, unchecked, to fwrite(), which takes a size_t as its argument. When your int value is converted to size_t (or treated as size_t), it becomes a rather large number (4 GB - 1 on a 32-bit system). So, fwrite() does as you command - tries to write a lot of data from places it is not supposed to.

Answer 4

You need to test the return value of read() - it may return an error code, such as -1, as well as the number of bytes read.

Edit: In your new code you should test that the return value from recv() is >= 0, not that it is not -1. You should alse break if it returns zero.

Answer 5

• fclose() returns a value: 0 if the file is closed successfully, EOF if not. Consider modifying your code to ...

  if (fclose(handle)) { printf("error closing file."); exit(-1); }

• As everyone else points out, check read() for error.

• If the crash is happening after the fclose returns 0 (success), and all the read()s were successful, then perhaps the trouble is elsewhere. Maybe it's the VLA. Consider changing the buffer code to a file static or a malloc / free.

• If the caller was mistaken about the file size, say, and claimed the file was 500 bytes when in fact it is only 480, will your code keep reading the socket forever?

Answer 6

Could it be because you are writing more bytes into the file than file_size ? Using the current logic if you have file_size as 1025 then you will end up in writing 2048 bytes to the file. I believe, you should recalculate the reading_unit inside the while loop.

Answer 7

I am unsure how the compiler will interpret these two lines

int SIZE = 1024;
char buffer[SIZE];

You are using a variable to size buffer rather than a constant. I am unsure whether this will behave as you expect. Could you try the following instead.

#define SIZE (1024)
char buffer[SIZE];

Answer 8

The code in edit3 looks reasonable. I suspect you've probably stomped over some memory somewhere else and are reaping the problem at this point in your application.

Do you have bounds checkers, purify etc that you could run to check for buffer overruns, using a free'd memory block etc.

Answer 9

Are you sure you haven't corrupted the heap elsewhere, before getting to this function? Try running your program under valgrind.

Answer 10

If your "EDIT3" version of the code is still crashing, then the problem lies elsewhere in your code. If you have made a mistake in another part of your program (for example, freeing something twice, dereferencing a pointer that wasn't pointing where you thought it was), then the resulting corruption can go unseen until later (such as when you reach fclose()).