By using this site, you agree to our updated Privacy Policy and our Terms of Use. Manage your Cookies Settings.
448,589 Members | 1,161 Online
Bytes IT Community
+ Ask a Question
Need help? Post your question and get tips & solutions from a community of 448,589 IT Pros & Developers. It's quick & easy.

Can any one help me find out what was the problem in this small

P: n/a
It always dumped when I tried to run it... But it compiles OK. What I
want to do is to do a test:
Read information from a .dat file and then write it to another file.
The original DAT file is like this : (very simple..........)
010001010110001101010101010101010101010101


#include<stdio.h>
#include<stdlib.h>
#include<string.h>

typedef struct _HEADER {

int num1;
int num2;
int num3;
int num4;

}HEADER;

int i;

int x;
int y;
int z;

void readheader(HEADER *,FILE *);
void writeheader(HEADER *,FILE *);
main()

{

FILE *fp1;
FILE *fp2;

HEADER header;
short ***datatable;

fp1=fopen("g.dat","rb+");
fp2=fopen("newdata.dat","wb+");
readheader(&header,fp1);
for(x=0; x<2 ;x++){
for (y=0; y<3; y++){
for(z=0; z<4; z++){
fread(&datatable[x][y][z],sizeof(short),24,fp1);
fclose(fp1);
}
}
}

writeheader(&header,fp2);

for(x=0; x<2 ;x++){
for (y=0; y<3; y++){
for(z=0; z<4; z++){
fwrite(&datatable[x][y][z],sizeof(short),24,fp2);
fclose(fp2);
}
}
}

}

void readheader(HEADER *header,FILE *fp1)
{
fread(&(header->num1),sizeof(int),1,fp1);
fread(&(header->num2),sizeof(int),1,fp1);
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);
}
Jul 20 '08 #1
Share this Question
Share on Google+
9 Replies


P: n/a
xiao wrote:
It always dumped when I tried to run it... But it compiles OK.
[...]
short ***datatable;
[no allocation anywhere of space for datatable (or *datatable or
**datatable) to point to, and then]
for(x=0; x<2 ;x++){
for (y=0; y<3; y++){
for(z=0; z<4; z++){
fread(&datatable[x][y][z],sizeof(short),24,fp1);
fclose(fp1);
}
And you actually wonder why you get segfault?
Jul 20 '08 #2

P: n/a
On 20 Jul, 03:57, xiao <littledd...@gmail.comwrote:
It always dumped when I tried to run it... But it compiles OK. *
<snip>
>
int i;
Why would you make your loop variables global? This
is a horrible decision.
>
main()
This declaration has been obsolete for a *long* time.
"int main(void)" or "int main( int argc, char **argv)".

>
fp1=fopen("g.dat","rb+");
fp2=fopen("newdata.dat","wb+");
You MUST check for errors here. If you wish to keep
the code simple for the sake of example, then just
write "fp1 = Fopen(..." and mention that Fopen s
a wrapper that checks for an error. Or, if you
prefer, don't mention that it is a wrapper and
let the reader make that assumption. But do not
ever fail to check the return value of a call
to fopen().

>
fread(&datatable[x][y][z],sizeof(short),24,fp1);

Since no memory has been allocated for datatable,
or datatable[x], or datatable[x][y],
this is very likely to cause an error. (You should
either malloc space for each, or simply declare
datatable as "short datatable[2][3][4]"). However,
it is not clear why you wish to read 24 shorts
24 times. If you do the loop, then just read one
value at a time. If you read 24 values, then don't
do the loop.

Jul 20 '08 #3

P: n/a
On Sat, 19 Jul 2008 19:57:59 -0700 (PDT), xiao <li*********@gmail.com>
wrote:
>It always dumped when I tried to run it... But it compiles OK. What I
want to do is to do a test:
Read information from a .dat file and then write it to another file.
The original DAT file is like this : (very simple..........)
010001010110001101010101010101010101010101


#include<stdio.h>
#include<stdlib.h>
#include<string.h>

typedef struct _HEADER {

int num1;
int num2;
int num3;
int num4;

}HEADER;

int i;

int x;
int y;
int z;

void readheader(HEADER *,FILE *);
void writeheader(HEADER *,FILE *);
main()

{

FILE *fp1;
FILE *fp2;

HEADER header;
short ***datatable;
Where does this pointer ever get assigned the address of memory you
own?
>
fp1=fopen("g.dat","rb+");
fp2=fopen("newdata.dat","wb+");
readheader(&header,fp1);
for(x=0; x<2 ;x++){
for (y=0; y<3; y++){
for(z=0; z<4; z++){
fread(&datatable[x][y][z],sizeof(short),24,fp1);
After you solve the problem of where datatable points to, this will
lead to a buffer overflow and undefined behavior. If datatable is
intended to be a 2 x 3 x 4 array of short, then there are only 24
short in the array. If fread is going to read all 24, is should do it
only once and it better start reading from the beginning.
fclose(fp1);
}
}
}

writeheader(&header,fp2);

for(x=0; x<2 ;x++){
for (y=0; y<3; y++){
for(z=0; z<4; z++){
fwrite(&datatable[x][y][z],sizeof(short),24,fp2);
fclose(fp2);
}
}
}

}

void readheader(HEADER *header,FILE *fp1)
{
fread(&(header->num1),sizeof(int),1,fp1);
fread(&(header->num2),sizeof(int),1,fp1);
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);
}

Remove del for email
Jul 20 '08 #4

P: n/a
On Jul 20, 2:04 am, Barry Schwarz <schwa...@dqel.comwrote:
On Sat, 19 Jul 2008 19:57:59 -0700 (PDT), xiao <littledd...@gmail.com>
wrote:
It always dumped when I tried to run it... But it compiles OK. What I
want to do is to do a test:
Read information from a .dat file and then write it to another file.
The original DAT file is like this : (very simple..........)
010001010110001101010101010101010101010101
#include<stdio.h>
#include<stdlib.h>
#include<string.h>
typedef struct _HEADER {
int num1;
int num2;
int num3;
int num4;
}HEADER;
int i;
int x;
int y;
int z;
void readheader(HEADER *,FILE *);
void writeheader(HEADER *,FILE *);
main()
{
FILE *fp1;
FILE *fp2;
HEADER header;
short ***datatable;

Where does this pointer ever get assigned the address of memory you
own?
fp1=fopen("g.dat","rb+");
fp2=fopen("newdata.dat","wb+");
readheader(&header,fp1);
for(x=0; x<2 ;x++){
for (y=0; y<3; y++){
for(z=0; z<4; z++){
fread(&datatable[x][y][z],sizeof(short),24,fp1);

After you solve the problem of where datatable points to, this will
lead to a buffer overflow and undefined behavior. If datatable is
intended to be a 2 x 3 x 4 array of short, then there are only 24
short in the array. If fread is going to read all 24, is should do it
only once and it better start reading from the beginning.
fclose(fp1);
}
}
}
writeheader(&header,fp2);
for(x=0; x<2 ;x++){
for (y=0; y<3; y++){
for(z=0; z<4; z++){
fwrite(&datatable[x][y][z],sizeof(short),24,fp2);
fclose(fp2);
}
}
}
}
void readheader(HEADER *header,FILE *fp1)
{
fread(&(header->num1),sizeof(int),1,fp1);
fread(&(header->num2),sizeof(int),1,fp1);
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);
}

Remove del for email
I changed something in the program but it still reminds that:
*** glibc detected *** double free or corruption (!prev): 0x0887f008
***
Abort (core dumped)
#include<stdio.h>
#include<stdlib.h>
#include<string.h>

typedef struct _HEADER {

int num1;
int num2;
int num3;
int num4;

}HEADER;

int i,j;

void **Allocate2D(long int NBytes, int rows, int columns);
/*here add a function*/
void readheader(HEADER *,FILE *);
void writeheader(HEADER *,FILE *);
main()

{

FILE *fp1;
FILE *fp2;

HEADER header;
short ****datatable;

fp1=fopen("g.dat","r");
fp2=fopen("newdata.dat","w");
readheader(&header,fp1);
datatable = (short ****)Allocate2D(sizeof(short **),2,2);

/*changed here*/

for (i=0;i<2;i++){
for(j=0; j<2; j++){
datatable[i][j] = (short **)Allocate2D(sizeof(short),3,4);
}
}

for(i=0; i<2 ;i++){
for (j=0; j<2; j++)fread(&datatable[i][j][0][0],sizeof(short),
12,fp1);
fclose(fp1);
}

writeheader(&header,fp2);

for(i=0; i<2 ;i++){
for (j=0; j<2; j++)fwrite(&datatable[i][j][0][0],sizeof(short),
12,fp2);
fclose(fp2);
}

/*read the file again*/

}

void readheader(HEADER *header,FILE *fp1)
{
fread(&(header->num1),sizeof(int),1,fp1);
fread(&(header->num2),sizeof(int),1,fp1);
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)
{
void **pntr;
int i;

pntr = (void **)malloc(sizeof(void *)*rows);
pntr[0] = (void *)malloc(NBytes*rows*columns);
for(i=1; i<rows; i++)pntr[i] = pntr[i-1]+(columns*NBytes);
return pntr;
}
/*new function*/
Jul 20 '08 #5

P: n/a
xiao said:

<snip>

fp1=fopen("g.dat","r");
fp2=fopen("newdata.dat","w");

You were already told why, after attempting to open a file, it's important
to check that the attempt was successful.

Since you've paid no attention to that excellent advice, why should people
bother to offer you any more?

--
Richard Heathfield <http://www.cpax.org.uk>
Email: -http://www. +rjh@
Google users: <http://www.cpax.org.uk/prg/writings/googly.php>
"Usenet is a strange place" - dmr 29 July 1999
Jul 20 '08 #6

P: n/a
In article <20**********************************@25g2000hsx.g ooglegroups.com>,
xiao <li*********@gmail.comwrote:
[reformatted for readability]
for (i=0; i<2; i++)
{
for (j=0; j<2; j++)
{
fwrite(&datatable[i][j][0][0],sizeof(short), 12, fp2);
}
fclose(fp2);
}
In the first iteration of the outer for loop (i.e. when i = 0),
fp2 is closed at the end of the iteration.
So, in the second iteration (when i = 1),
fwrite is called with an invalid FILE pointer.

Ike
Jul 20 '08 #7

P: n/a
On Jul 20, 1:42 pm, i...@localhost.claranet.nl (Ike Naar) wrote:
In article <20eceb7a-950c-4667-b88e-51d17874f...@25g2000hsx.googlegroups.com>,

xiao <littledd...@gmail.comwrote:
[reformatted for readability]
for (i=0; i<2; i++)
{
for (j=0; j<2; j++)
{
fwrite(&datatable[i][j][0][0],sizeof(short), 12, fp2);
}
fclose(fp2);
}

In the first iteration of the outer for loop (i.e. when i = 0),
fp2 is closed at the end of the iteration.
So, in the second iteration (when i = 1),
fwrite is called with an invalid FILE pointer.

Ike
Thank you guys~ I tried to do some modification and it seems that the
file does not open correcly.
It reminds me that

"The processing file is ="
WHEN I tried to use command line arguments in it.
Jul 20 '08 #8

P: n/a
On Sun, 20 Jul 2008 09:13:19 -0700 (PDT), xiao <li*********@gmail.com>
wrote:
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[i][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[i][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[i][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[i] = 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.
return pntr;
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.
>}
/*new function*/

Remove del for email
Jul 20 '08 #9

P: n/a
On 20 Jul, 17:13, xiao <littledd...@gmail.comwrote:
void **Allocate2D(long int NBytes, int rows, int columns);

*datatable = (short ****)Allocate2D(sizeof(short **),2,2);
Allocate2D returns a void **. It is highly unlikely
that it is usable as a short ****. I haven't looked yet,
but I am already willing to bet that you have made
mistakes in Allocate2D.

>
void **Allocate2D(long int NBytes, int rows, int columns)
{
* * *void **pntr;
* * *int i;

* * *pntr * *= (void **)malloc(sizeof(void **)*rows);
* * *pntr[0] = (void *)malloc(NBytes*rows*columns);
* * *for(i=1; i<rows; i++)pntr[i] = pntr[i-1]+(columns*NBytes);
* * *return pntr;}
It would be a lot easier if Allocate2D simply returns a short **.
(I don't understand why you've declared datatable a short ****,
since a 2 dimensional array of shorts is typically of type
short **. Your previous code appeared to want a 3 dimensional
array, in which case short *** would be appropriate.)
If you want a 2-dimensional array, you will need to allocate
space to hold a pointer to each row, and then allocate space
for each row. If you only want one allocation, and you
intend to determine the base of each row yourself, then
you don't want a short ** at all, but simply a short *.
For example:

short * two_d_array;
short ** row_ptrs;

two_d_array = xmalloc( rows * columns * sizeof *two_d_array );
row_ptrs = xmalloc( rows * sizeof *row_ptrs );
for( r = 0; r < rows; r++ )
row_ptrs[ r ] = xmalloc( columns * sizeof **row_ptrs );
(xmalloc is a wrapper around malloc that aborts on error.)

BTW, you also really do need to check the return value of
fclose() as well. If you are using fwrite, it is entirely possible
that fwrite will succeed and no error will occur until
after main returns and the streams are flushed. Test
your program (by attempting to write to /dev/null, for
example) and make sure your program generates a useful
warning message.

Jul 21 '08 #10

This discussion thread is closed

Replies have been disabled for this discussion.