Segfault with dynamically created 2-D array

[Ovid]

Hi all,

I'm having a problem trying to create a 2D array whose dimensions are
determined at runtime. Below my signoff is a minimal test case that
hopefully demonstrates what I'm trying to do. Unfortunately, this
segfaults. The output is the following:

$ gcc -Wall -c arrays.c
$ gcc -o arrays arrays.o
$ ./arrays
0, 0 is 0.790188
0, 1 is 0.344383
0, 2 is 0.733099
1, 0 is 0.748440
1, 1 is 0.861647
1, 2 is 0.147551
2, 0 is 0.285223
2, 1 is 0.718230
2, 2 is 0.227775
3, 0 is 0.503970
3, 1 is 0.427397
3, 2 is 0.578871
Segmentation fault

Note: This is the first pass of this problem, but the next pass will
require turning this into a 3 dimensional array with none of the
parameters known at compile time. I assume this has no bearing on the
problem, but I'm not sure.

Any critique of the code is welcome as it's been years since I've
worked with C.

Cheers,
Curtis "Ovid" Poe

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

double **weights;

double ** malloc_weights(int num_rows, int rowsize)
{
int i;

/* The last element is 0, so free_weights can detect the last row
*/
weights = malloc(sizeof(void *) * (num_rows+2)); /* one extra for
sentinel */
if(weights == 0) return 0;

/* allocate the actual rows */
for(i = 0; i < num_rows; i++) {
weights[i] = malloc(rowsize);
if(weights[i] == 0) {
return 0;
}
}

/* initialize the sentinel value */
weights[num_rows+1] = 0;

return weights;
}

void assign_random_weights(int rows, int cols)
{
int i,j;

for (i = 0; i < rows+1; i++) {
for (j = 0; j < cols; j++) {
weights[i][j] = ( ((float)rand() / (float)RAND_MAX) -.05
);
printf("%d, %d is %f\n", i, j, weights[i][j]);
}
}
}

int main(void)
{
double **array;
int i,j;
int rows = 4;
int cols = 3;

array = malloc_weights(rows,cols*sizeof(double));
assign_random_weights(rows,cols);

for (i = 0; i < rows; i++)
for (j = 0; j < cols; j++)
printf("%d, %d is %f\n", i, j, weights[i][j]);
return 0;
}

[Artie Gold]

Ovid wrote:

Hi all,

I'm having a problem trying to create a 2D array whose dimensions are
determined at runtime. Below my signoff is a minimal test case that
hopefully demonstrates what I'm trying to do. Unfortunately, this
segfaults. The output is the following:

$ gcc -Wall -c arrays.c
$ gcc -o arrays arrays.o
$ ./arrays
0, 0 is 0.790188
0, 1 is 0.344383
0, 2 is 0.733099
1, 0 is 0.748440
1, 1 is 0.861647
1, 2 is 0.147551
2, 0 is 0.285223
2, 1 is 0.718230
2, 2 is 0.227775
3, 0 is 0.503970
3, 1 is 0.427397
3, 2 is 0.578871
Segmentation fault

Note: This is the first pass of this problem, but the next pass will
require turning this into a 3 dimensional array with none of the
parameters known at compile time. I assume this has no bearing on the
problem, but I'm not sure.

Any critique of the code is welcome as it's been years since I've
worked with C.

Cheers,
Curtis "Ovid" Poe

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

double **weights;

double ** malloc_weights(int num_rows, int rowsize)
{
int i;

/* The last element is 0, so free_weights can detect the last row
*/
weights = malloc(sizeof(void *) * (num_rows+2)); /* one extra for
sentinel */
Well, that's two extra, but who's counting. ;-)
The following would be better:

weights = malloc(sizeof *weights * (num_rows + 1));

(Question: why sizeof(void *) in your original?)
if(weights == 0) return 0;

/* allocate the actual rows */
for(i = 0; i < num_rows; i++) {
weights[i] = malloc(rowsize);
Nope. Not enough room. Try:

weights[i] = malloc(sizeof *(weights[i]) * rowsize);
if(weights[i] == 0) {
return 0;
}
}

/* initialize the sentinel value */
weights[num_rows+1] = 0;
You really want:
weights[num_rows] = NULL;

return weights;
}

void assign_random_weights(int rows, int cols)
{
int i,j;

for (i = 0; i < rows+1; i++) {
for (j = 0; j < cols; j++) {
weights[i][j] = ( ((float)rand() / (float)RAND_MAX) -.05
);
printf("%d, %d is %f\n", i, j, weights[i][j]);
}
}
}

int main(void)
{
double **array;
int i,j;
int rows = 4;
int cols = 3;

array = malloc_weights(rows,cols*sizeof(double));
You need to check that the allocation in `malloc_weights()' succeeded.
assign_random_weights(rows,cols);

for (i = 0; i < rows; i++)
for (j = 0; j < cols; j++)
printf("%d, %d is %f\n", i, j, weights[i][j]);
return 0;
}

HTH,
--ag

--
Artie Gold -- Austin, Texas

[Eric Sosman]

Ovid wrote:

Hi all,

I'm having a problem trying to create a 2D array whose dimensions are
determined at runtime. [...]

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

double **weights;

double ** malloc_weights(int num_rows, int rowsize)
{
int i;

/* The last element is 0, so free_weights can detect the last row
*/
weights = malloc(sizeof(void *) * (num_rows+2)); /* one extra for
sentinel */
First mistake (which you may be getting away with).
`weights' is a `double**', that is, a pointer to a `double*'.
A `double*' is not a `void*', and may possibly have a
different size (although on many machines their sizes are
identical, which is why you may not have been hurt -- yet).
You should write `sizeof(double*)' instead of `sizeof(void*)'.
Even better, write `sizeof *weights' and let the compiler
figure it out.
if(weights == 0) return 0;

/* allocate the actual rows */
for(i = 0; i < num_rows; i++) {
weights[i] = malloc(rowsize);

Second mistake (which is almost certainly biting you).
`weights[i]' is a `double*', a pointer to `double'. It is
almost certainly the case that `sizeof(double)' is greater
than one, yet you're allocating only one byte per element
instead of `sizeof(double)' bytes per element. Remember
how you multipled by a `sizeof' (albeit the wrong one) in
the previous allocation? You must also do so here:

weights[i] = malloc(rowsize * sizeof *weights[i]);

I didn't study the rest of your program for further
errors, but these two are already enough to doom you. Fix
them first, and see what happens.

--
Er*********@sun.com

[Barry Schwarz]

On 19 Sep 2003 09:21:42 -0700, po**@yahoo.com (Ovid) wrote:

Hi all,

I'm having a problem trying to create a 2D array whose dimensions are
determined at runtime. Below my signoff is a minimal test case that
hopefully demonstrates what I'm trying to do. Unfortunately, this
segfaults. The output is the following: snip sample output#include <stdio.h>
#include <stdlib.h>

double **weights;

double ** malloc_weights(int num_rows, int rowsize)
As is evident from the previous responses, your practice of including
sizeof(double) in the calculation of rowsize is sufficiently uncommon
to confuse people. That alone is a reasonable argument for not doing
so.

However, it is not the cause of your problem. Keep reading.
{
int i;

/* The last element is 0, so free_weights can detect the last row
*/
weights = malloc(sizeof(void *) * (num_rows+2)); /* one extra for
sentinel */
Here you allocate space for 6 pointers. Others have pointed out that
sizeof(void*) is not what you wanted. Since sizeof(double*) usually
has the same value, it will normally not lead to the segfault you
experience.
if(weights == 0) return 0;

/* allocate the actual rows */
for(i = 0; i < num_rows; i++) {
weights[i] = malloc(rowsize);
if(weights[i] == 0) {
return 0;
}
}
Here you have initialized four of the six pointers, specifically
weights[0], [1], [2], and [3].

/* initialize the sentinel value */
weights[num_rows+1] = 0;
Here you initialize a fifth pointer, weights[5]. Note that the sixth
pointer, weights[4], is never initialized.

return weights;
}

void assign_random_weights(int rows, int cols)
{
int i,j;

for (i = 0; i < rows+1; i++) {
Here you try to process five rows, 0, 1, 2, 3, and 4.
for (j = 0; j < cols; j++) {
weights[i][j] = ( ((float)rand() / (float)RAND_MAX) -.05
As soon as i becomes 4, this statement invokes undefined behavior
since weights[4] is still uninitialized. This is what causes your
segfault.
);
printf("%d, %d is %f\n", i, j, weights[i][j]);
}
}
}

int main(void)
{
double **array;
int i,j;
int rows = 4;
int cols = 3;

array = malloc_weights(rows,cols*sizeof(double));
assign_random_weights(rows,cols);

for (i = 0; i < rows; i++)
for (j = 0; j < cols; j++)
printf("%d, %d is %f\n", i, j, weights[i][j]);
return 0;
}

<<Remove the del for email>>

[Al Bowers]

Ovid wrote:

Hi all,

I'm having a problem trying to create a 2D array whose dimensions are
determined at runtime. Below my signoff is a minimal test case that
hopefully demonstrates what I'm trying to do. Unfortunately, this
segfaults. The output is the following:

$ gcc -Wall -c arrays.c
$ gcc -o arrays arrays.o
$ ./arrays
0, 0 is 0.790188
0, 1 is 0.344383
0, 2 is 0.733099
1, 0 is 0.748440
1, 1 is 0.861647
1, 2 is 0.147551
2, 0 is 0.285223
2, 1 is 0.718230
2, 2 is 0.227775
3, 0 is 0.503970
3, 1 is 0.427397
3, 2 is 0.578871
Segmentation fault

Note: This is the first pass of this problem, but the next pass will
require turning this into a 3 dimensional array with none of the
parameters known at compile time. I assume this has no bearing on the
problem, but I'm not sure.

Any critique of the code is welcome as it's been years since I've
worked with C.

Not a bad start but you have several problems that need to
be corrected. The most serious errors are that you are not
allocating enough space and you have a serious memory leak
should the allocations fail. Why return the global variable?
Actually, why do you declare the global variable weights?
Cheers,
Curtis "Ovid" Poe

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

double **weights;

double ** malloc_weights(int num_rows, int rowsize)
{
int i;
weights = malloc(sizeof(void *) * (num_rows+2)); /* one extra for
sentinel */
weights = malloc((sizeof *weights)*(num_rows+1));
if(weights == 0) return 0;

/* allocate the actual rows */
for(i = 0; i < num_rows; i++) {
weights[i] = malloc(rowsize);
weights[i] = malloc((sizeof **weights)*rowsize):
if(weights[i] == 0) {
TODO: To prevent a memory leak that could occur, free up all
previously allocated space if an allocation error occurs here.

return 0; }
}

/* initialize the sentinel value */
weights[num_rows+1] = 0;

return weights;
}

void assign_random_weights(int rows, int cols)
{
int i,j;

for (i = 0; i < rows+1; i++) {
for (j = 0; j < cols; j++) {
weights[i][j] = ( ((float)rand() / (float)RAND_MAX) -.05
);
printf("%d, %d is %f\n", i, j, weights[i][j]);
}
}
}

int main(void)
{
double **array;
int i,j;
int rows = 4;
int cols = 3;

array = malloc_weights(rows,cols*sizeof(double));
assign_random_weights(rows,cols);

for (i = 0; i < rows; i++)
for (j = 0; j < cols; j++)
printf("%d, %d is %f\n", i, j, weights[i][j]);
return 0;
}

corrected:

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

double **malloc_weights(int num_rows, int rowsize)
{
int i;
double **dd;

dd = malloc((sizeof *dd)*(num_rows+1));/* extra sentinel value*/
if(dd == NULL) return NULL;
/* allocate the actual rows */
for(i = 0; i < num_rows; i++)
{
dd[i] = malloc((sizeof **dd) * rowsize);
if(dd[i] == NULL)
{
for(i-- ; i >= 0; i--) free(dd[i]);
free(dd);
return NULL;
}
}
/* initialize the sentinel value */
dd[num_rows] = NULL;
return dd;
}

void assign_random_weights(double **array, int rows, int cols)
{
int i,j;

for (i = 0; i < rows; i++)
for (j = 0; j < cols; j++)
array[i][j] = (((float)rand()/(float)RAND_MAX)-.05);
}

int main(void)
{
double **array;
int i,j;
int rows = 4;
int cols = 3;

array = malloc_weights(rows,cols*sizeof(double));
if(array)
{
assign_random_weights(array, rows,cols);
for (i = 0; i < rows; i++)
for (j = 0; j < cols; j++)
printf("%d, %d is %f\n", i, j, array[i][j]);

/* free the array */
for(i = 0;i < rows;i++) free(array[i]);
free(array);
}
return 0;
}

--
Al Bowers
Tampa, Fl USA
mailto: xa*@abowers.combase.com (remove the x)
http://www.geocities.com/abowers822/