-1

I am not understanding whats erroneous in the program. I am defining a pointer to an array of structures. Malloc'ed enough memory for it. Initialized the array elements. Then used fwrite to write the array on to a binary file. Then attempting to read the same, back into another pointer to a similar array, which has enough memory malloc'ed to it. 

#include<stdio.h>

typedef struct ss{
int *p;
char c;
double d;
char g;
float f;
} dd;

main(){

dd (*tt)[5];
int i=0,a[5]={4,1,6,9,3};
tt=malloc(sizeof(struct ss[5]));
for(i=0;i<5;i++){
   tt[i]->p=malloc(sizeof(int));
   tt[i]->p=&a[i];
   tt[i]->c=(char)('a'+i);
   tt[i]->d=(double)(5.234234+i);
   tt[i]->g=(char)('A'+i);
   tt[i]->f=(float)(15.234234+i);
}

FILE *F;
F=fopen("myfile","w+b");
size_t l;
l=fwrite(tt,sizeof(*tt),1,F);
fseek(F,0,SEEK_SET);
//printf("sizeof(dd)=%d   sizeof(*tt) =%d bytes written %d\n",sizeof(dd),sizeof(*tt),l);

dd (*xx)[5];

xx=malloc(sizeof(struct ss[5]));
l=fread(xx,sizeof(*xx),1,F);

for(i=0;i<5;i++){
printf("%d, %c,%f,%c,%f\n",*(xx[i]->p),xx[i]->c,xx[i]->d,xx[i]->g,xx[i]->f);
}
printf("Date Read %d \n",l);
for(i=0;i<5;i++){
free(xx[i]->p);
}
free(xx);
free(tt);
fclose(F);
remove("myfile");
}

Output:
4,a,5.234234,A,15.234234
Segmentation fault

Pkp
  • 929
  • 6
  • 19
  • 31

2 Answers2

1

You weren't writing your data where you thought you were, because you were accessing tt incorrectly. Your incorrect access was consistent, and therefore you could read out the first record, but the second record was nowhere near where you thought it was- it was, in fact, being written into uninitialized memory and never saved. Trying to access the reloaded data shows this. Additionally, your int* in your struct couldn't be written out correctly the way you wrote it out, but this is moot because of how your program is structured- it would be wrong if you were trying to load the file in a separate run of the program. fwrite and fread cannot follow your int*, because it's only looking at your struct as a bit pattern- it is faithfully reconstructing your pointer, but now you have a pointer to a random chunk of memory that you didn't actually do anything with! In this case, though, your pointers remain valid because you never overwrote the data, but this is specific to the scenario of writing the file out, not flushing the memory, and reading it back in without the program being closed- which is not a realistic scenario for file-writing. There's another StackOverflow question that explains this bug in more detail.

Anyway, here's the much bigger problem with how you're accessing memory, with other lines removed:

dd (*tt)[5];
//...
tt=malloc(sizeof(struct ss[5]));
for(i=0;i<5;i++){
   tt[i]->p=malloc(sizeof(int));
   tt[i]->p=&a[i];
   //...
}

C declarations are read with The Clockwise Spiral Rule, so let's look at what we've said about tt and compare it to how we're using it.

tt is the variable name. To its right is a closing parenthesis, so we keep processing the current scope. We encounter a *, and then the matching paren, then a static array size, then a type. Using The Clockwise Spiral Rule, tt is a pointer to an array (size 5) of dd. This means that if you dereference tt (using (*tt)), you get a dd[5], or, if you prefer to think of it that way (C certainly does), a pointer to the beginning of a block of memory large enough to hold your structure. More importantly, that's what you've said it is. C isn't actually very picky about is pointer types, and that's why your code compiles even though you're committing a serious type error.

Your malloc statement is correct: it is initializing tt with a memory location that the operating system promised has enough space for your five ss. Because C doesn't bother with silly things like array size bounds checking, a 5-element array of struct ss is guaranteed to be exactly five times the size of a single struct ss, so you could actually have written malloc(5 * sizeof(dd)), but either way of writing it is fine.

But let's look at what happens here:

tt[i]->p=malloc(sizeof(int));

Uh-oh. tt is a pointer to an array of struct dd, but you've just treated it as an array of pointers to struct dd.

What you wanted:

  • Dereference tt
  • Find the ith element in an array of pointers to dd
  • Go to field p
  • Assign it a pointer to space for an int

What you actually got:

  • Find the ith element in an array of pointers to arrays of dd
  • Dereference it, treating it as a pointer to dd, since C doesn't know the difference between arrays and pointers
  • Go indirectly to field p
  • Assign it a pointer to space for an int

When i is 0, this works properly, because the zeroth element in an array and the array itself are in the same location. (An array has no header, and C _does not understand the difference between arrays and pointers and allows you to use them interchangeably, which is why this compiles at all.)

When i is not 0, you make an enormous mess of memory. Now you are writing to whatever memory happened to follow your pointer! It's actually a pointer, but you told C it was an array, and it believed you, added 1 element-width to its location, and tried to do all those operations. You're using arrays exactly where you should be using pointers, and pointers exactly where you should be using arrays.

You only write to the memory you allocated for element 0. Beyond that, you're writing into unrelated memory, and it's luck (bad luck, in your case) that kept your program from crashing right there. (If it had, you'd have had an easier time finding this as the guilty line.) When you fwrite your allocated memory, the first element is valid, the rest is garbage, and your fread results in a data structure that has one valid element, then random heap garbage that resulted in a crash when you tried to dereference a pointer (that would only be valid because the program didn't end).

Here's the right way to access your pointer-to-array:

(*tt)[i].p=malloc(sizeof(int));...

Also, you're allocating memory, then immediately forgetting your only reference to it, which is a memory leak, since you're overwriting the pointer with a reference to the static array you're initializing everything with. Use this instead:

*((*tt)[i].p)=a[i]

I strongly encourage you to study A Tutorial on Pointers and Arrays in its entirety. It will help you avoid this class of issue in the future.

Be aware that you are reading xx incorrectly when printing its contents in exactly the same manner.

Community
  • 1
  • 1
Adam Norberg
  • 3,028
  • 17
  • 22
  • I knew that writing pointers to a file is a bad idea. I was doing this exercise only to understand pointer to arrays better. You made it very clear thanks. I will read that Array's and pointer link. +1 on the mem leak catch. I changed the code where the pointers just hold the address of the static array now. – Pkp Mar 21 '12 at 14:38
0

You're pointer usage is incorrect. In this code snippet:

dd (*xx)[5];

xx=malloc(sizeof(struct ss[5]));
l=fread(xx,sizeof(*xx),1,F);

for(i=0;i<5;i++){
printf("%d, %c,%f,%c,%f\n",*(xx[i]->p),xx[i]->c,xx[i]->d,xx[i]->g,xx[i]->f);
}

You are declaring xx as a pointer to an array of 5 'dd' structures. This is where it gets weird. It's a pointer to five structures and not an array of five structures.

It would look something like this in memory:

dd[0] = [{p, c, d, g, f}, {p, c, d, g, f}, {p, c, d, g, f}, {p, c, d, g, f}, {p, c, d, g, f}]
dd[1] = [{p, c, d, g, f}, {p, c, d, g, f}, {p, c, d, g, f}, {p, c, d, g, f}, {p, c, d, g, f}]
...
dd[4] = [{p, c, d, g, f}, {p, c, d, g, f}, {p, c, d, g, f}, {p, c, d, g, f}, {p, c, d, g, f}]

Instead of the intended:
dd[0] = {p, c, d, g, f}
dd[1] = {p, c, d, g, f}
...
dd[4] = {p, c, d, g, f}

As you are iterating from 0 to 5, each array access is advancing your array sizeof(ss[5]) bytes in memory instead of sizeof(ss) bytes. Take out the extra pointer.

dd* xx;
xx = (dd*)malloc(sizeof(dd) * 5);
l = fread(xx, sizeof(dd), 5, F);

for(i = 0; i < 5; ++i) {
  printf("%d, %c, %f, %c, %f\n", xx[i].p, , xx[i].c, xx[i].d, xx[i].g, xx[i].f);
}

Additionally you have a problem with your structure. If it is meant to be directly written to disk like this, it cannot contain pointers. Thus your 'int *p;' member needs to instead be 'int p;'. Otherwise if you read this file from a separate application, the pointer you stored will not point at the integer anymore, but at unallocated memory.

Writing application:
    int *p = 0x12345 ---> 5
0x12345 gets stored in the file for p.

Writing application reads the file.
    int *p = 0x12345 ---> 5
The pointer still points at the same memory because it is still the same memory
  layout.

New application reads the file.
    int *p = 0x12345 ---> ?????
The pointer doesn't point to a known piece of memory because the memory layout
  has changed in this new instance of the application. This could crash or
  cause a security issue.
Avilo
  • 1,204
  • 7
  • 7