Memory Corruption Problem

[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

--
I am the "ILOVEGNU" signature virus. Just copy me to your
signature. This email was infected under the terms of the GNU
General Public License.

[Xavier]

August Karlstrom wrote:

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; 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 *)); 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]; 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

Xavier

[Xavier]

Xavier wrote:

August Karlstrom wrote:

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;
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 *));

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];

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

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

Xavier

[Pedro Graca]

August Karlstrom wrote:

$ cat test.c [snip] res->at = malloc(rows * sizeof (double *));
res->at = malloc(rows * sizeof *(res-at));
for (j = 0; j < rows; j++) {
res->at[j] = malloc(cols * sizeof (double *));
res->at[j] = malloc(cols * sizeof *(res->at[j]));
} [snip]
Any clues?

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>

--
If you're posting through Google read <http://cfaj.freeshell.org/google>

[Jack Klein]

On 1 Mar 2006 02:17:02 GMT, Pedro Graca <he****@dodgeit.com> wrote in
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>

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.

--
Jack Klein
Home: http://JK-Technology.Com
FAQs for
comp.lang.c http://c-faq.com/
comp.lang.c++ http://www.parashift.com/c++-faq-lite/
alt.comp.lang.learn.c-c++
http://www.contrib.andrew.cmu.edu/~a...FAQ-acllc.html

[August Karlstrom]

Xavier wrote:

August Karlstrom wrote:

[snip]

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

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

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 *));

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

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];

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

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 am the "ILOVEGNU" signature virus. Just copy me to your
signature. This email was infected under the terms of the GNU
General Public License.

[Ian Collins]

Pedro Graca wrote:

You should also free the allocated memory before exiting.

<OT>
If not free'd by the program will the OS free it? 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. </OT>

--
Ian Collins.

[August Karlstrom]

Xavier wrote:

Xavier wrote:

[snip]

res->at[j][k] = elements[j * rows + k];

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

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

--
I am the "ILOVEGNU" signature virus. Just copy me to your
signature. This email was infected under the terms of the GNU
General Public License.

[Keith Thompson]

August Karlstrom <fu********@comhem.se> writes:

Xavier wrote:

August Karlstrom wrote: [snip]

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

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

or just `typedef struct {} Matrix;'

Or just "struct matrix { ... };" and don't bother with the typedef.
;-) A style issue.

Indeed.

--
Keith Thompson (The_Other_Keith) ks***@mib.org <http://www.ghoti.net/~kst>
San Diego Supercomputer Center <*> <http://users.sdsc.edu/~kst>
We must do something. This is something. Therefore, we must do this.

[Pedro Graca]

Pedro Graca wrote:

res->at = malloc(rows * sizeof *(res-at));

Oops :( ^^^^^^

I meant this instead
res->at = malloc(rows * sizeof *(res->at));

--
If you're posting through Google read <http://cfaj.freeshell.org/google>

[August Karlstrom]

Pedro Graca wrote:

August Karlstrom wrote: [snip]

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

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

Thanks.
After a couple errors like that I always do

POINTER = malloc(ELEMS * sizeof *POINTER);
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>

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

--
I am the "ILOVEGNU" signature virus. Just copy me to your
signature. This email was infected under the terms of the GNU
General Public License.

[August Karlstrom]

Keith Thompson wrote:

August Karlstrom <fu********@comhem.se> writes:

Xavier wrote:

August Karlstrom wrote:

[snip]

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

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

or just `typedef struct {} Matrix;'

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

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

--
I am the "ILOVEGNU" signature virus. Just copy me to your
signature. This email was infected under the terms of the GNU
General Public License.

[August Karlstrom]

Pedro Graca wrote:

After a couple errors like that I always do

POINTER = malloc(ELEMS * sizeof *POINTER);

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

--
I am the "ILOVEGNU" signature virus. Just copy me to your
signature. This email was infected under the terms of the GNU
General Public License.

[Keith Thompson]

August Karlstrom <fu********@comhem.se> writes:

Pedro Graca wrote:

After a couple errors like that I always do
POINTER = malloc(ELEMS * sizeof *POINTER);

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

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

--
Keith Thompson (The_Other_Keith) ks***@mib.org <http://www.ghoti.net/~kst>
San Diego Supercomputer Center <*> <http://users.sdsc.edu/~kst>
We must do something. This is something. Therefore, we must do this.

[websnarf]

August Karlstrom wrote:

[...]
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 *));

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.

--
Paul Hsieh
http://www.pobox.com/~qed/
http://bstring.sf.net/

[Vladimir S. Oka]

we******@gmail.com wrote:

August Karlstrom wrote:

[...]
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 *));
You've got a units error here. It should be:

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

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).

<snip>
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.

Really, now!

--
BR, Vladimir

[mybirth1983]

hi

[Vladimir S. Oka]

[e-mail] wrote:

hi

lo

[August Karlstrom]

we******@gmail.com wrote:

August Karlstrom wrote:

[...]
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 *));

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.

Thanks for finding the bug and for the other hints.
August

--
I am the "ILOVEGNU" signature virus. Just copy me to your
signature. This email was infected under the terms of the GNU
General Public License.

[Barry Schwarz]

On Wed, 01 Mar 2006 03:42:28 GMT, August Karlstrom
<fu********@comhem.se> wrote:

Pedro Graca wrote:

After a couple errors like that I always do

POINTER = malloc(ELEMS * sizeof *POINTER);

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

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