Yes, you are having a problem with your use of pointer arithmetic, but
there are a few other issues as well...
Hello,
I'm trying to allocate an array inside a function and read data from a file
into it. I can read data into the first element of the array fine, but I'm
quite confused as to how to read data into the second element. Code below:
int get_mft(FILE *f, unsigned int blocksize, MFTENTRY **mft)
{
int MFTEntries;
*mft = (MFTENTRY*) malloc(sizeof(MFTENTRY));
Danger! Alert! Asking for trouble starting with the line above...
It is never necessary to cast the value returned by malloc() in C.
The conversion from void pointer to any type of object pointer is
automatic and requires no cast.
If your compiler complains about assigning the return value to a
pointer, that means you have forgotten to include <stdlib.h> with the
proper prototype for malloc().
And never, Never, NEVER use the pointer returned by malloc() without
first testing for NULL.
Also you almost never need to apply the sizeof operator to the name of
a type when using a memory allocation or file read/write function,
unless you are doing some sort of type punning.
//read MFT to get size of MFT
Do you have a C99 conforming compiler? There are very, very few of
them. If not, comments beginning with // are not legal.
fread(*mft, sizeof(MFTENTRY), 1, f);
Never use the contents of an area of memory after an input operation
without checking the result of the input operation first. See my
sample code at the end.
MFTEntries = blocks_to_bytes(mft[0]->blockend[0] - mft[0]->blockstart[0],
blocksize) / sizeof(MFTENTRY);
//read rest of MFT
realloc(*mft, MFTEntries * sizeof(MFTENTRY));
You're asking for a memory leak by passing your original pointer to
realloc() without making a temporary copy first. If the realloc()
fails, it will return NULL. The original memory is still allocated
but you no longer have a pointer to it to free it.
And of course, never use the value returned by malloc(), calloc(), or
realloc() without first checking for NULL.
fread(*mft+sizeof(MFTENTRY), sizeof(MFTENTRY), MFTEntries-1, f); //crashes
here
Here's where pointer arithmetic is clobbering you. When you add an
offset to a pointer, the compiler automatically scales the offset by
the size of the type pointed to.
The expression "*mft+sizeof(MFENTRY)" is not equivalent to the second
in an array of these structures. If the size of the MFENTRY type is
50 bytes, for example, this expression is equivalent to accessing the
50th element of an array of (at least) 50 structures.
If you want to access the second element of an array via a pointer,
you want *mft+1.
return MFTEntries;
}
It _seems_ to work fine if I pass &mft[0][1] to the second fread(), until I
try to free() mft. BTW, it's not a "real" filesystem, just something I
thought I'd do for fun.
Please set your newsreader to add a proper signature to your posts. A
proper signature line consists of "-- \n", that is two minus signs, a
single blank space, and then an end of line. See mine below for an
example. News software automatically recognizes that pattern and
trims the signature when replying.
Personally I would never dereference a pointer to something as small
as a structure pointer that many times. If you reference it that many
times it is just as efficient, if not more so, to make a local copy on
most implementations. Even more than any slight gain in efficiency,
it makes the code much more readable.
Try this on for size:
int get_mft(FILE *f, unsigned int blocksize, MFTENTRY **mft)
{
int MFTEntries = 0; /* if unsuccessful */
int number_read;
MFTENTRY *mft_real, *mft_temp;
*mft = NULL; /* in case the function fails */
mft_real = malloc(sizeof *mft_real); /* don't need sizeof(type) */
if (NULL == mft_real)
{
return 0;
}
number_read = fread(mft_real, sizeof *mft_real, 1, f);
if (1 != number_read)
{
free(mft_real);
return 0;
}
MFTEntries = blocks_to_bytes(mft_real->blockend[0] -
mft_real->blockstart[0], blocksize / sizeof *mft_real);
mft_temp = realloc(mft_real, MFTEntries * sizeof *mft_real);
if (NULL == mft_temp)
{
free(mft_real);
return 0;
}
mft_real = mft_temp;
number_read = fread(mft_real + 1, sizeof *mft_real,
MFTEntries - 1, f);
if (MFTEntries - 1 != f)
{
free(mft_real);
return 0;
}
*mft = mft_real;
return MFTEntries;
}
Note:
1. How much more straight forward the code is without the repeated
dereference to the pointer-to-pointer. And probably at least as
efficient on most platforms.
2. The pointer arithmetic in the second fread() call, just add 1!
3. At no point is the sizeof operator applied to the name of a type.
As long as you have a pointer to the type around, you can do sizeof
*pointer_to_type. Even if the pointer is not initialized or NULL,
because sizeof does not evaluate its operand.
4. No cast on malloc() or realloc(), but make sure <stdlib.h> is
included.
--
Jack Klein
Home:
http://JK-Technology.Com
FAQs for
comp.lang.c
http://www.eskimo.com/~scs/C-faq/top.html
comp.lang.c++
http://www.parashift.com/c++-faq-lite/
alt.comp.lang.learn.c-c++ ftp://snurse-l.org/pub/acllc-c++/faq
