Memory Corruption Problem

  • Thread starter August Karlstrom
  • Start date
A

August Karlstrom

Hi,

Inspired by the recent post by "questions?" I started to write a simple
matrix library. My problem is that when I run the program below I get
unexpected results.

$ cat test.c
#include <stdio.h>
#include <stdlib.h>

#define SIZE 2

struct matrix
{
int rows, cols;
double **at;
};
typedef struct matrix Matrix;

Matrix *new_matrix(int rows, int cols, const double *elements)
{
Matrix *res;
int j, k;

res = malloc(sizeof (Matrix));
res->rows = rows;
res->cols = cols;
res->at = malloc(rows * sizeof (double *));
for (j = 0; j < rows; j++) {
res->at[j] = malloc(cols * sizeof (double *));
}
if (elements != NULL) {
for (j = 0; j < rows; j++) {
for (k = 0; k < cols; k++) {
res->at[j][k] = elements[j * cols + k];
}
}
}
return res;
}


int main(void)
{
Matrix *A, *B;
double a[SIZE * SIZE] = {1.0, 2.0, 3.0, 4.0};

A = new_matrix(SIZE, SIZE, a);
printf("%f\n", A->at[1][1]);
B = new_matrix(SIZE, SIZE, a);
printf("%f\n", A->at[1][1]);
return 0;
}
$ gcc -ansi -pedantic -std=c89 -Wall -g test.c module.c -o test
$ ./test
4.000000
0.000000

I expect the output to be

4.000000
4.000000

Any clues?


Regards,

August
 
X

Xavier

August said:
Hi,

Inspired by the recent post by "questions?" I started to write a simple
matrix library. My problem is that when I run the program below I get
unexpected results.

$ cat test.c
#include <stdio.h>
#include <stdlib.h>

#define SIZE 2

struct matrix
{
int rows, cols;
double **at;
};
typedef struct matrix Matrix;
Click to expand...
or all in one typedef struct matrix {...} Matrix;
Matrix *new_matrix(int rows, int cols, const double *elements)
{
Matrix *res;
int j, k;

res = malloc(sizeof (Matrix));
res->rows = rows;
res->cols = cols;
res->at = malloc(rows * sizeof (double *));
for (j = 0; j < rows; j++) {
res->at[j] = malloc(cols * sizeof (double *));
Click to expand...
res->at[j] = malloc(cols * sizeof (double));
}
if (elements != NULL) {
for (j = 0; j < rows; j++) {
for (k = 0; k < cols; k++) {
res->at[j][k] = elements[j * cols + k];
Click to expand...
sure ? it seems to be incoherent...
res->at[j][k] = elements[j * rows + k];
}
}
}
return res;
}


int main(void)
{
Matrix *A, *B;
double a[SIZE * SIZE] = {1.0, 2.0, 3.0, 4.0};

A = new_matrix(SIZE, SIZE, a);
printf("%f\n", A->at[1][1]);
B = new_matrix(SIZE, SIZE, a);
printf("%f\n", A->at[1][1]);
return 0;
}
$ gcc -ansi -pedantic -std=c89 -Wall -g test.c module.c -o test
$ ./test
4.000000
0.000000

I expect the output to be

4.000000
4.000000

Any clues?


Regards,

August
Click to expand...

Xavier
 
X

Xavier

Xavier said:
August said:
Hi,

Inspired by the recent post by "questions?" I started to write a
simple matrix library. My problem is that when I run the program below
I get unexpected results.

$ cat test.c
#include <stdio.h>
#include <stdlib.h>

#define SIZE 2

struct matrix
{
int rows, cols;
double **at;
};
typedef struct matrix Matrix;
Click to expand...

or all in one typedef struct matrix {...} Matrix;
Matrix *new_matrix(int rows, int cols, const double *elements)
{
Matrix *res;
int j, k;

res = malloc(sizeof (Matrix));
res->rows = rows;
res->cols = cols;
res->at = malloc(rows * sizeof (double *));
for (j = 0; j < rows; j++) {
res->at[j] = malloc(cols * sizeof (double *));
Click to expand...

res->at[j] = malloc(cols * sizeof (double));
}
if (elements != NULL) {
for (j = 0; j < rows; j++) {
for (k = 0; k < cols; k++) {
res->at[j][k] = elements[j * cols + k];
Click to expand...

sure ? it seems to be incoherent...
res->at[j][k] = elements[j * rows + k];
Click to expand...
you should put *values* in your array, isn't it?
*(res->at[j]+k) = *(elements+j * rows + k);
}
}
}
return res;
}


int main(void)
{
Matrix *A, *B;
double a[SIZE * SIZE] = {1.0, 2.0, 3.0, 4.0};

A = new_matrix(SIZE, SIZE, a);
printf("%f\n", A->at[1][1]);
B = new_matrix(SIZE, SIZE, a);
printf("%f\n", A->at[1][1]);
return 0;
}
$ gcc -ansi -pedantic -std=c89 -Wall -g test.c module.c -o test
$ ./test
4.000000
0.000000

I expect the output to be

4.000000
4.000000

Any clues?


Regards,

August
Click to expand...

Xavier
Click to expand...
 
P

Pedro Graca

August said:
$ cat test.c [snip]
res->at = malloc(rows * sizeof (double *));
Click to expand...

res->at = malloc(rows * sizeof *(res-at));
for (j = 0; j < rows; j++) {
res->at[j] = malloc(cols * sizeof (double *));
Click to expand...

res->at[j] = malloc(cols * sizeof *(res->at[j]));
} [snip]

Any clues?
Click to expand...


After a couple errors like that I always do

POINTER = malloc(ELEMS * sizeof *POINTER);



I tried your code as you posted it and got the
4.000000
0.000000
output. With the corrections noted above the output was what you
expected.


You should also free the allocated memory before exiting.

<OT>
If not free'd by the program will the OS free it?
</OT>
 
J

Jack Klein

comp.lang.c:

[snip]

You should also free the allocated memory before exiting.

<OT>
If not free'd by the program will the OS free it?
</OT>
Click to expand...

Some operating systems do, some do not. Most modern ones do, there
were certainly some older ones that did not.

The C standard has no control over operating systems, even operating
systems on which one might wish to execute C programs. So it cannot
impose requirements on what an operating system does before or after a
C program executes.

An early language decision, eventually codified in the standard, was
that normal termination of a program (return from main() or a call to
exit()) will close all open files, which includes flushing any
buffered data to the OS.

Another very specific decision was that normal program termination
would not be required to free allocated memory, and so most
implementations do not.

There is nothing preventing an implementation from walking its memory
allocation tables and freeing any still allocated memory on normal
termination. This is assuming that the implementation has retained
sufficient bookkeeping information to do so.

But once an implementation yields control back to the platforms by
returning a termination status, neither the executable nor the C
standard controls what the system does.
 
A

August Karlstrom

Xavier said:
August Karlstrom wrote: [snip]
struct matrix
{
int rows, cols;
double **at;
};
typedef struct matrix Matrix;
Click to expand...

or all in one typedef struct matrix {...} Matrix;
Click to expand...

or just `typedef struct {} Matrix;' ;-) A style issue.
Matrix *new_matrix(int rows, int cols, const double *elements)
{
Matrix *res;
int j, k;

res = malloc(sizeof (Matrix));
res->rows = rows;
res->cols = cols;
res->at = malloc(rows * sizeof (double *));
for (j = 0; j < rows; j++) {
res->at[j] = malloc(cols * sizeof (double *));
Click to expand...

res->at[j] = malloc(cols * sizeof (double));
Click to expand...

Yes, there was the bug. Thanks.
}
if (elements != NULL) {
for (j = 0; j < rows; j++) {
for (k = 0; k < cols; k++) {
res->at[j][k] = elements[j * cols + k];
Click to expand...

sure ? it seems to be incoherent...
res->at[j][k] = elements[j * rows + k];
Click to expand...

No, `elements' contain the matrix rows (this should of course be
documented). There are j * cols elements above the row with index j.
Then we add k to get to the column.


August
 
I

Ian Collins

Pedro said:
You should also free the allocated memory before exiting.

<OT>
If not free'd by the program will the OS free it?
Click to expand...
Well that depends on the OS! But yes, a typical OS will, otherwise
you'd have a big problem with memory leeks when programs exit abnormally.
 
A

August Karlstrom

Xavier said:
Xavier wrote: [snip]
res->at[j][k] = elements[j * rows + k];
Click to expand...

you should put *values* in your array, isn't it?
*(res->at[j]+k) = *(elements+j * rows + k);
Click to expand...

Different syntax, same semantics. I find subscripts less obscure.


August
 
A

August Karlstrom

Pedro said:
August Karlstrom wrote: [snip]
res->at[j] = malloc(cols * sizeof (double *));
Click to expand...


res->at[j] = malloc(cols * sizeof *(res->at[j]));
Thanks.

After a couple errors like that I always do

POINTER = malloc(ELEMS * sizeof *POINTER);
Click to expand...

Yes, this is good practice unless POINTER is a very long expression. It
would be excellent if there was a LINT tool (static type checker) that
could detect errors like the one I made. At least SPLINT won't.
You should also free the allocated memory before exiting.

<OT>
If not free'd by the program will the OS free it?
</OT>
Click to expand...

I rely on that. I think all modern OS:s do.


August
 
A

August Karlstrom

Keith said:
August Karlstrom said:
Xavier said:
August Karlstrom wrote:
[snip]

struct matrix
{
int rows, cols;
double **at;
};
typedef struct matrix Matrix;

or all in one typedef struct matrix {...} Matrix;
Click to expand...

or just `typedef struct {} Matrix;'
Click to expand...


Or just "struct matrix { ... };" and don't bother with the typedef.
Click to expand...

IMHO this leads to overly verbose variable/parameter/field declarations.


August
 
A

August Karlstrom

Pedro said:
After a couple errors like that I always do

POINTER = malloc(ELEMS * sizeof *POINTER);
Click to expand...

So the C standard says it's OK to dereference an uninitialized pointer
in `sizeof' expressions, right?


August
 
K

Keith Thompson

August Karlstrom said:
So the C standard says it's OK to dereference an uninitialized pointer
in `sizeof' expressions, right?
Click to expand...

Yes. The C standard says that the operand of sizeof is not evaluated
unless it's of variable length array type.
 
W

websnarf

August said:
[...]
Matrix *new_matrix(int rows, int cols, const double *elements) {
Matrix *res;
int j, k;

res = malloc(sizeof (Matrix));
res->rows = rows;
res->cols = cols;
res->at = malloc(rows * sizeof (double *));
for (j = 0; j < rows; j++) {
res->at[j] = malloc(cols * sizeof (double *));
Click to expand...

You've got a units error here. It should be:

res->at[j] = (double *) malloc (cols * sizeof (double));

The way to think about this is that the lowest level always is an array
of the base primitive. Another way is to realize that the units for
your first malloc is the same as the malloc for this one -- so
something must be wrong, since they are allocating different things.
You can make a macro like this:

#define arrAlloc(type,qty) (type *) malloc ((qty) * sizeof (type))

This will give you some semblance of type safety, as well as automatic
units checking:

res->at[j] = arrAlloc (double, cols);

And for the earlier allocation:

res->at = arrAlloc (double *, 1);

So you are not going to both a type mistmatch error, and a units error.
If you are worried about missing #include <stdlib.h> error, use a C++
compiler.
 
V

Vladimir S. Oka

August said:
[...]
Matrix *new_matrix(int rows, int cols, const double *elements) {
Matrix *res;
int j, k;

res = malloc(sizeof (Matrix));
res->rows = rows;
res->cols = cols;
res->at = malloc(rows * sizeof (double *));
for (j = 0; j < rows; j++) {
res->at[j] = malloc(cols * sizeof (double *));
Click to expand...

You've got a units error here. It should be:

res->at[j] = (double *) malloc (cols * sizeof (double));
Click to expand...

No. Never cast what `malloc` returns, it may hide errors (like not
including stdlib.h).

It should be:

res->at[j] = malloc (res->cols * sizeof ( *(res->at[j]) ) );

This has added benefit of guarding you from change in the type (think
complex numbers).

said:
So you are not going to both a type mistmatch error, and a units error.
If you are worried about missing #include <stdlib.h> error, use a C++
compiler.
Click to expand...

Really, now!
 
A

August Karlstrom

August said:
[...]
Matrix *new_matrix(int rows, int cols, const double *elements) {
Matrix *res;
int j, k;

res = malloc(sizeof (Matrix));
res->rows = rows;
res->cols = cols;
res->at = malloc(rows * sizeof (double *));
for (j = 0; j < rows; j++) {
res->at[j] = malloc(cols * sizeof (double *));
Click to expand...


You've got a units error here. It should be:

res->at[j] = (double *) malloc (cols * sizeof (double));

The way to think about this is that the lowest level always is an array
of the base primitive. Another way is to realize that the units for
your first malloc is the same as the malloc for this one -- so
something must be wrong, since they are allocating different things.
You can make a macro like this:

#define arrAlloc(type,qty) (type *) malloc ((qty) * sizeof (type))

This will give you some semblance of type safety, as well as automatic
units checking:

res->at[j] = arrAlloc (double, cols);

And for the earlier allocation:

res->at = arrAlloc (double *, 1);

So you are not going to both a type mistmatch error, and a units error.
If you are worried about missing #include <stdlib.h> error, use a C++
compiler.
Click to expand...

Thanks for finding the bug and for the other hints.


August
 
B

Barry Schwarz

So the C standard says it's OK to dereference an uninitialized pointer
in `sizeof' expressions, right?
Click to expand...

In this case, sizeof does not evaluate its operand so the pointer is
not dereferenced. It is not OK to dereference an uninitialized
pointer but that is not happening here.


Remove del for email
 

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments. After that, you can post your question and our members will help you out.

Ask a Question

Similar Threads

Fibonacci 0
Could you fix my code please? (i get no output after inputing the number) 1
C program: memory leak/ segmentation fault/ memory limit exceeded 0
Java matrix problem 3
Difference between using "let" in a "for" loop 0
Need Help: Program to Accept 2 Matrices and Show their Sum 0
Matrix multiplication code 6
Memory corruption on freeing a pointer to pointer 172

Members online

No members online now.
Total: 32 (members: 0, guests: 32)
Robots: 329

Forum statistics

Threads
473,755
Messages
2,569,536
Members
45,007
Latest member
obedient dusk

Latest Threads

Top