views:

1716

answers:

4

I'm trying to read a file into a buffer in blocks of size BLOCK_SIZE (currently equal to 1000 unsigned chars). My code first finds the number of blocks it will have to read in order to read the entire file (usually 2-4), then iterates through a for loop reading the file (ignore the "+17+filenamesize" stuff, that is all needed for later in the program.

However, only on the first time through, when j=1, does it actually put data into the buf array. In other cases, when j != 1, strlen(buf) returns 0.

I think it is either a problem with how I'm using fseek() to seek to the second part of a file before reading it, or perhaps a memory allocation issue.

Any help would be appreciated for getting it to read the 1000-1999th chars of the file into the buf array.

Attached is the relevant part of the code:

unsigned char *buf;
source = fopen(localpath,"r");
temp = filesize / BLOCK_SIZE + 1;

for (j=1; j <= temp; j++) {
  if (j == 1) {
     buf = (unsigned char *) malloc((sizeof(unsigned char)) * (BLOCK_SIZE + 17 + filenamesize));
     fread(buf+17+filenamesize, sizeof(unsigned char), BLOCK_SIZE, source);
   } else if (j == temp) {
     buf = (unsigned char *) malloc((sizeof(unsigned char)) * (filesize + 5 - BLOCK_SIZE*(j-1)));
     fseek(source, BLOCK_SIZE*(j-1), SEEK_SET); // off by one warning
     fread(buf+5, sizeof(unsigned char), filesize - BLOCK_SIZE*(j-1), source);
   } else {
     buf = (unsigned char *) malloc((sizeof(unsigned char)) * (5+BLOCK_SIZE*(j-1)));
     fseek(source, BLOCK_SIZE*(j-1), SEEK_SET); // off by one warning
     fread(buf+5, sizeof(unsigned char), BLOCK_SIZE, source);
   }
   // do stuff with buf here

   buf = "";
   free(buf);
}
A: 

srtlen returns the length of a string (number of bytes before the first 0 byte). If buf[0] is 0 it returns 0. Use the return value of the fread to determine how many bytes are actually read.

You also have memory leak. You malloc in every loop iteration but only free once at the end.

stribika
He calls free inside the loop - that's not his problem....
Reed Copsey
You are correct. There is no leak.
stribika
There is a leak - he assigns buf to a static pointer ("") and then tries to free() it. That shouldn't be working in the first place.
Chris Lutz
+4  A: 

I would recommend checking the results of fseek and fread. In particular, make sure fseek is returning 0 - if it's not, this may be the entire problem.

Provided that fseek is succeeding, fread should tell you the total number of bytes read.

Also, strlen is not necessarily a valid thing to use, since it's going to assume that this is a null terminated string. If the first character you read is a 0 byte, strlen will return 0. You're not treating this as a null terminated string (you aren't allocating enough space for teh null terminator - just exactly what's needed to fit your binary data), so strlen is probably inappropriate.

Reed Copsey
Thanks, that was indeed the problem, that the string started with null characters. That has been fixed, and I also made it null-terminated, and now it is working fine!
Logan Williams
+1  A: 

The line buf = ""; looks like a bug to me. This will set the pointer buf to a constant string which you also try to free() on the next line. I would just skip this line.

You also seem to read into the buffer with some offsets. ie +5 in the two later cases. The first part in the buffer will then be undefined, see man page for malloc. So a strlen(buf) feels undefined for me.

epatel
So there is a leak after all. Only the "" string is freed. I think that can crash.
stribika
Thanks for pointing out that leak -- I've fixed it now.
Logan Williams
A: 

Why are you using fseek at all? The whole notion of "first check how big the file is to determine how many times to read a block" is fundamentally flawed. You should simply read data until there is no more data left, for example:

while( BLOCK_SIZE == ( read_count = fread( buf, sizeof *buf, count, source ))
    do_stuff_with_buf( buf, read_count );

if( ferror( source ))
    /* Handle error */;

(This example will never call do_stuff_with_buf() on a short read, but that is a trivial modification.)

William Pursell