Memory Corruption Problem

Discussion in 'C Programming' started by August Karlstrom, Mar 1, 2006.

  1. 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.
     
    August Karlstrom, Mar 1, 2006
    #1
    1. Advertising

  2. August Karlstrom

    Xavier Guest

    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, Mar 1, 2006
    #2
    1. Advertising

  3. August Karlstrom

    Xavier Guest

    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
     
    Xavier, Mar 1, 2006
    #3
  4. August Karlstrom

    Pedro Graca Guest

    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>
     
    Pedro Graca, Mar 1, 2006
    #4
  5. August Karlstrom

    Jack Klein Guest

    On 1 Mar 2006 02:17:02 GMT, Pedro Graca <> 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/~ajo/docs/FAQ-acllc.html
     
    Jack Klein, Mar 1, 2006
    #5
  6. 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.
     
    August Karlstrom, Mar 1, 2006
    #6
  7. August Karlstrom

    Ian Collins Guest

    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.
     
    Ian Collins, Mar 1, 2006
    #7
  8. 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.
     
    August Karlstrom, Mar 1, 2006
    #8
  9. August Karlstrom <> 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) <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.
     
    Keith Thompson, Mar 1, 2006
    #9
  10. August Karlstrom

    Pedro Graca Guest

    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>
     
    Pedro Graca, Mar 1, 2006
    #10
  11. 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, Mar 1, 2006
    #11
  12. Keith Thompson wrote:
    > August Karlstrom <> 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, Mar 1, 2006
    #12
  13. 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.
     
    August Karlstrom, Mar 1, 2006
    #13
  14. August Karlstrom <> 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) <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.
     
    Keith Thompson, Mar 1, 2006
    #14
  15. August Karlstrom

    Guest

    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/
     
    , Mar 1, 2006
    #15
  16. 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
     
    Vladimir S. Oka, Mar 1, 2006
    #16
  17. August Karlstrom

    Guest

    hi
     
    , Mar 1, 2006
    #17
  18. wrote:
    > hi


    lo
     
    Vladimir S. Oka, Mar 1, 2006
    #18
  19. 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.
     
    August Karlstrom, Mar 1, 2006
    #19
  20. On Wed, 01 Mar 2006 03:42:28 GMT, August Karlstrom
    <> 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
     
    Barry Schwarz, Mar 12, 2006
    #20
    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. Noa Garnett

    memory corruption while debugging

    Noa Garnett, Aug 24, 2004, in forum: C++
    Replies:
    7
    Views:
    1,817
    Howard
    Aug 25, 2004
  2. Gavin Kreuiter

    Debugger "print" clears memory corruption

    Gavin Kreuiter, Dec 4, 2003, in forum: C Programming
    Replies:
    3
    Views:
    437
    Chris Torek
    Dec 5, 2003
  3. detecting memory corruption

    , Dec 26, 2004, in forum: C Programming
    Replies:
    9
    Views:
    696
    Keith Thompson
    Dec 29, 2004
  4. Sune
    Replies:
    14
    Views:
    895
    Chris Thomasson
    Aug 26, 2007
  5. Sune
    Replies:
    5
    Views:
    455
    Darko
    Jul 13, 2007
Loading...

Share This Page