snip 100+ lines of old code
If you are going to rework the program, there is no point in requoting
all the irrelevant old code.
I changed something in the program but it still reminds that:
*** glibc detected *** double free or corruption (!prev): 0x0887f008
***
Abort (core dumped)
Since you have no calls to free in your program (something else to be
"fixed"), you must be writing to storage beyond the end of some
allocated space. It's not obvious in the code but after you fix the
allocation function things may be easier to spot.
You could try stepping through the code with a debugger.
#include<stdio.h>
#include<stdlib.h>
#include<string.h>
typedef struct _HEADER {
int num1;
Indenting is useful for declarations also.
int num2;
int num3;
int num4;
}HEADER;
int i,j;
Why are these global?
void **Allocate2D(long int NBytes, int rows, int columns);
While void* is a "generic" object pointer, void** is not. Since you
intend for the return value of this function to be assigned to a
short****, this can cause all kinds of problems with alignments and
sizes.
/*here add a function*/
void readheader(HEADER *,FILE *);
void writeheader(HEADER *,FILE *);
main()
Use int main(void) if want others to compile your code as part of
helping you.
{
FILE *fp1;
FILE *fp2;
HEADER header;
short ****datatable;
fp1=fopen("g.dat","r");
fp2=fopen("newdata.dat","w");
Calls to fopen should always be tested for success.
How is it you use excessive vertical white space and so little
horizontal? fp2 = fopen is so much easier to read.
readheader(&header,fp1);
datatable = (short ****)Allocate2D(sizeof(short **),2,2);
The presence of the cast should be the first clue that your are doing
something wrong. See the comments in the function.
Isn't it your intent to use the values obtained by readheader instead
of the constants?
If we assume the type issues in the function has been fixed and a
typical system (2 byte short and 4 byte pointers), you first allocate
8 bytes (e.g., 0x10000 - 0x10007). You then allocate 16 more
bytes(e.g., 0x11000 - 0x1100f). When the function returns:
datatable contains 0x10000
datatable[0] located at 0x10000 contains 0x11000
datatable[1] located at 0x10004 contains 0x11008
Continued at the next call...
/*changed here*/
for (i=0;i<2;i++){
for(j=0; j<2; j++){
datatable[j] = (short **)Allocate2D(sizeof(short),3,4);
....continued here. For i and j 0: You first allocate 12 bytes (e.g.,
0x12000 - 0x1200b). You then allocate 24 bytes (e.g., 0x13000 -
0x13017). When the function returns:
datatable[0][0] located at 0x11000 contains 0x12000
datatable[0][0][0] located at 0x12000 contains 0x13000
datatable[0][0][1] located at 0x12004 contains 0x13008
datatable[0][0][2] located at 0x12008 contains 0x13010
It looks like there should be enough room for the upcoming fread.
}
}
for(i=0; i<2 ;i++){
for (j=0; j<2; j++)fread(&datatable[j][0][0],sizeof(short),
12,fp1);
It's usually a good idea to check input operations for success also.
fclose(fp1);
}
writeheader(&header,fp2);
for(i=0; i<2 ;i++){
for (j=0; j<2; j++)fwrite(&datatable[j][0][0],sizeof(short),
12,fp2);
fclose(fp2);
}
/*read the file again*/
Useless comments are bad enough. Incorrect ones just add to the
confusion.
}
void readheader(HEADER *header,FILE *fp1)
{
fread(&(header->num1),sizeof(int),1,fp1);
fread(&(header->num2),sizeof(int),1,fp1);
Well done. Since there is no guarantee the four members of the struct
are adjacent, reading each individually is the best approach. Two
points of style:
-> binds more tightly than & so the parentheses around
header->num2 are superfluous. &header->num2 is exactly the same.
At some point you may need to change the type of num2. If you
do, you will also have to change the sizeof(int) argument. You can
make the code "self adjusting" by changing the argument to
sizeof header->num2
or
sizeof(header->num2)
Even though the parentheses are unnecessary, I prefer to use them when
the operand of sizeof is a long expression with additional embedded
operators.
fread(&(header->num3),sizeof(int),1,fp1);
fread(&(header->num4),sizeof(int),1,fp1);
}
void writeheader(HEADER *header,FILE *fp2)
{
fwrite(&(header->num1),sizeof(int),1,fp2);
fwrite(&(header->num2),sizeof(int),1,fp2);
fwrite(&(header->num3),sizeof(int),1,fp2);
fwrite(&(header->num4),sizeof(int),1,fp2);
}
void **Allocate2D(long int NBytes, int rows, int columns)
The return type is useless for this program. The only purpose of
void** is to point to a void*. Nowhere else in your program do you
use any void* and you use the one in this function incorrectly
{
void **pntr;
int i;
pntr = (void **)malloc(sizeof(void *)*rows);
NEVER cast the return from malloc.
All allocation calls should always be checked for success.
pntr[0] = (void *)malloc(NBytes*rows*columns);
ESPECIALLY NEVER cast it to the same type it already is.
for(i=1; i<rows; i++)pntr = pntr[i-1]+(columns*NBytes);
This statement contains a constraint violation. Your compiler is
required to produce a diagnostic. Did it? If so why did you ignore
it? (You should not attempt to execute your program until you have a
clean compile.) If it did not produce a diagnostic, either up the
warning level and disengage extensions or get a better compiler.
Let's look at this function in detail as it relates to the first call
from main. For sake of discussion, let's assume
sizeof(short) is 2
sizeof(short*) is 4
sizeof(short**) is 4
sizeof(short***) is 4
sizeof(short****) is 4
sizeof(void**) is 8
sizeof(void*) is 12
(Yes, these are perverse values but they serve to illustrate the
point.)
The first call to malloc allocates space for 2 void*. If malloc
returns 0x10000, then the first void* is there and the second is at
0x1000c.
The second call to malloc allocates space for 4 short**. If malloc
returns 0x11000, then the first short**is there and the next is at
0x11004 and then 0x11008 and the last at 0x1100c
The for loop will execute exactly once. However, there is no way for
it to compute the value to be assigned to pntr[1]. pntr is a void**.
Therefore pntr[0] is a void*. You cannot do arithmetic on a void*
because the type it points to (namely void) is an incomplete type that
has no size. For purposes of pointer arithmetic, some compiler
extensions (which should be avoided in this group) treat void* as a
char*. In this case, pntr[1] which is located at 0x1000c will be
assigned the value 0x11008.
The return attempts to pass the value 0x10000 as a void** back to main
where it will be cast to a short****. BUT void** is an 8 byte object
and short**** is a 4 byte object and there is no guarantee that it
will fit. This is potentially undefined behavior. There is also no
guarantee that the value of a void** is properly aligned for a
short****. More potentially undefined behavior.
Now look what happens in main. datatable contains the value 0x10000.
datatable is a short****. That means it points to a short***. A
short*** is 4 bytes. Therefore, the short*** it points to (known as
datatable[0]) occupies the bytes from 0x10000 to ox10003 and
datatable[1] starts at 0x10004. But that is not what Alloc2D did. It
stored the two values at 0x10000 through 0x1000b and 0x1000c through
0x10017. More undefined behavior.
Enough academics. Your system may be insensitive to these issues with
short being 2 bytes and int and pointers being 4. But you should be
aware of them. There is no reason to expose your code to these
potential problems. Think about what may happen if you ever upgrade
to a 64 bit system.
Remove del for email