Segfault with dynamically created 2-D array

Discussion in 'C Programming' started by Ovid, Sep 19, 2003.

  1. Ovid

    Ovid Guest

    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 = malloc(rowsize);
    if(weights == 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[j] = ( ((float)rand() / (float)RAND_MAX) -.05
    );
    printf("%d, %d is %f\n", i, j, weights[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[j]);
    return 0;
    }
     
    Ovid, Sep 19, 2003
    #1
    1. Advertising

  2. Ovid

    Artie Gold Guest

    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 = malloc(rowsize);


    Nope. Not enough room. Try:

    weights = malloc(sizeof *(weights) * rowsize);

    > if(weights == 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[j] = ( ((float)rand() / (float)RAND_MAX) -.05
    > );
    > printf("%d, %d is %f\n", i, j, weights[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[j]);
    > return 0;
    > }


    HTH,
    --ag

    --
    Artie Gold -- Austin, Texas
     
    Artie Gold, Sep 19, 2003
    #2
    1. Advertising

  3. Ovid

    Eric Sosman Guest

    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 = malloc(rowsize);


    Second mistake (which is almost certainly biting you).
    `weights' 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 = malloc(rowsize * sizeof *weights);

    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.

    --
     
    Eric Sosman, Sep 19, 2003
    #3
  4. On 19 Sep 2003 09:21:42 -0700, (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 = malloc(rowsize);
    > if(weights == 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[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[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[j]);
    > return 0;
    >}




    <<Remove the del for email>>
     
    Barry Schwarz, Sep 20, 2003
    #4
  5. Ovid

    Al Bowers Guest

    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 = malloc(rowsize);


    weights = malloc((sizeof **weights)*rowsize):

    > if(weights == 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[j] = ( ((float)rand() / (float)RAND_MAX) -.05
    > );
    > printf("%d, %d is %f\n", i, j, weights[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[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 = malloc((sizeof **dd) * rowsize);
    if(dd == NULL)
    {
    for(i-- ; i >= 0; i--) free(dd);
    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[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[j]);

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

    --
    Al Bowers
    Tampa, Fl USA
    mailto: (remove the x)
    http://www.geocities.com/abowers822/
     
    Al Bowers, Sep 20, 2003
    #5
    1. Advertising

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

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Replies:
    0
    Views:
    306
  2. msimmons
    Replies:
    0
    Views:
    516
    msimmons
    Jul 16, 2009
  3. Andrey Vul
    Replies:
    8
    Views:
    699
    Richard Bos
    Jul 30, 2010
  4. Anthony Martinez
    Replies:
    4
    Views:
    287
    Robert Klemme
    Jun 11, 2007
  5. Michal Suchanek
    Replies:
    6
    Views:
    242
    Nobuyoshi Nakada
    Jun 13, 2007
Loading...

Share This Page