Function call makes const attribute of a struct change its value

Question

I'm writing this simple library to deal with int matrices:

#ifndef MATRIX_H_
#define MATRIX_H_

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

typedef struct Matrix Matrix;

Matrix* newMatrix(const unsigned int rows, const unsigned int columns);

void setElementAt(Matrix* self, const unsigned int row, const unsigned int column, const int value);

int getElementAt(const Matrix* self, const unsigned int row, const unsigned int column);

int getRowsNo(const Matrix* self);

int getColumnsNo(const Matrix* self);

void initMatrix(Matrix* self, int value);

#endif

#include "Matrix.h"

struct Matrix {

    int* grid;
    const unsigned int rowsNo;
    const unsigned int columnsNo;
};

Matrix* newMatrix(const unsigned int rowsNo, const unsigned int columnsNo) {

    assert(rowsNo > 0 && columnsNo > 0);

    Matrix new = {

        .grid = malloc(rowsNo * columnsNo * sizeof(int)),
        .rowsNo = rowsNo,
        .columnsNo = columnsNo
    };

    Matrix* self = &new;

    return self;
}

int getRowsNo(const Matrix* self) {

    return self->rowsNo;
}

int getColumnsNo(const Matrix* self) {

    return self->columnsNo;
}

int getElementAt(const Matrix* self, const unsigned int row, const unsigned int column) {

    assert(row < self->rowsNo && column < self->columnsNo);

    return self->grid[row * self->rowsNo + column];
}

void setElementAt(Matrix* self, const unsigned int row, const unsigned int column, const int value) {

    assert(row < self->rowsNo && column < self->columnsNo);

    self->grid[row * self->rowsNo + column] = value;
}

void initMatrix(Matrix* self, int value) {

    for(int row = 0; row < self->rowsNo; row++) {

        for(int column = 0; column < self->columnsNo; column++) {

            setElementAt(self, row, column, value);
        }
    }
}

The problem I'm having is that each time that the functions getElementAt() or setElementAt() are invoked, the columnsNo field (and only that one) of the struct Matrix instances changes to a huge random value, despite it being marked as const. Which problem am I failing to see here?

What do you think happens to `Matrix new` when your `newMatrix()` function returns? — Andrew Henle, Mar 05 '20 at 15:14
@Gian The function newMatri returns pointer to a local variable that will not alive after exiting the function. So the program has undefined behavior. — Vlad from Moscow, Mar 05 '20 at 15:15

yano · Accepted Answer · 2020-03-05T15:43:03.443

4

In newMatrix, You're returning the address of the local variable new. Once the function returns self, new goes out of scope and self doesn't point anywhere. Dereferencing that pointer invokes undefined behavior. You need to dynamically allocate memory using malloc or a related function to have it persist outside of newMatrix.

This is tagged c++, but the exact same concept applies to c: Can a local variable's memory be accessed outside its scope?

This is one approach to fix this problem

Matrix* newMatrix(const unsigned int rowsNo, const unsigned int columnsNo) {

    assert(rowsNo > 0 && columnsNo > 0);

    Matrix* new = malloc(sizeof *new);
    if (new != NULL)
    {
      new->grid = malloc(rowsNo * columnsNo * sizeof(int));
      if (new->grid == NULL) { /* handle error */ }
      new->rowsNo = rowsNo;
      new->columnsNo = columnsNo;
    }

    return new;
    // on return, the caller should check `newMatrix` returned a valid pointer.
}

Note that the assignments of new->rowsNo and new->columnsNo will not work as shown above since they are const. One way to assign these is by using memcpy, seen in the top answer here: How to initialize const members of structs on the heap

edited Mar 05 '20 at 15:43

answered Mar 05 '20 at 15:15

yano

4,827
2
23
35

Didn't know about that, since the compiler didn't complain about it, I just naively assumed it was okay to do this. Thank you all for the quick replies, I really couldn't figure out what was going on. I will keep this in mind for the future. – Gian Mar 05 '20 at 15:21
that's actually how I usually do this sort of stuff. The reason I tried this other approach (which I never had used before, that's why I was so baffled about it) it's because this way the initialization of .rowsNo and .columnsNo isn't possible if they are set to const. I just randomly tried to do this to fix this problem and it seemed to work fine. Should I assume that there's no valid way to have const attributes in structs if I intend to use them exclusively as pointers? – Gian Mar 05 '20 at 15:33
@Gian I'm a bit surprised there isn't a warning for that, I know I've see a warning for returning the address of a local variable before. At least this version of gcc doesn't seem to be issuing it though: https://godbolt.org/z/iTbfH2 – yano Mar 05 '20 at 15:34
@Gian Good question about the constness, I noticed that too while playing in godbolt. I don't know the answer without doing some research, maybe somebody else here will know. – yano Mar 05 '20 at 15:36
1

@Gian looks like you need to `memcpy`: https://stackoverflow.com/questions/2219001/how-to-initialize-const-members-of-structs-on-the-heap – yano Mar 05 '20 at 15:38
1

Honestly, yes, it showed up, and I was just about to give up on this approach. But then I tried to assign the address of that local variable to a newly declared pointer to Matrix. That got rid of the warning and the address was actually returned by the function (while just returning the variable called "new" wouldn't even do that). https://godbolt.org/z/Ncqwg9 Here the program actually returns 255, on my compiler (I'm using minGW by the way) it would basically just ignore what I was doing, aside from giving me those warnings. – Gian Mar 05 '20 at 15:41
@Gian Ahhhh, I see. The `self = &new` must be just enough indirection for the compiler to miss that `self` is actually pointing to a local. It's perfectly valid to return a pointer of course, so it appears here the compiler writers didn't want to deal with the complexity of trying to figure out if a returned pointer points to a local. It's "easy" enough to issue the warning when explicitly returning the address of a local. – yano Mar 05 '20 at 15:48
1

Honestly, as I already said, this was really naive on my end. Should've guessed that doing something like this wasn't safe at all. By the way, thank you for your teaching, they were really inforamtive :) – Gian Mar 05 '20 at 16:03

score 2 · Answer 2 · answered Mar 05 '20 at 15:30

This function

Matrix* newMatrix(const unsigned int rowsNo, const unsigned int columnsNo) {

    assert(rowsNo > 0 && columnsNo > 0);

    Matrix new = {

        .grid = malloc(rowsNo * columnsNo * sizeof(int)),
        .rowsNo = rowsNo,
        .columnsNo = columnsNo
    };

    Matrix* self = &new;

    return self;
}

is a reason of undefined behavior because the function returns pointer to the local object new with the automatic storage duration

    Matrix* self = &new;

    return self;

that will not be alive after exiting the function. So the returned pointer has an invalid value that does not point to a valid object.

In fact there is no sense to return from the function a pointer. You can just return the created object. Also the data member grid should be zero-initialized.

The function can look the following way

Matrix  newMatrix(const unsigned int rowsNo, const unsigned int columnsNo) {

    assert(rowsNo > 0 && columnsNo > 0);

    Matrix m = {
        .grid = calloc( rowsNo * columnsNo, sizeof(int) ),
        .rowsNo = rowsNo,
        .columnsNo = columnsNo
    };

    return m;
}

And in the caller you can write

Matrix m = newMatrix( rowsNo, columnsNo );

The functions getElementAt and setElementAt have a bug.

Instead of the expression

row * self->rowsNo + column

You have to use

row * self->columnsNo + column

Also you need to write a function that will free thr allocated memory for the data member grid.

Just curious why not return a pointer here: `Matrix *newMatrix(const unsigned int rowsNo, const unsigned int columnsNo)` ?? — ryyker, Mar 05 '20 at 15:50
@ryyker Here is created a matrix. It is not allocated dynamically. There is no need to do so. Moreover in the original question the matrix also is nit created dynamically. — Vlad from Moscow, Mar 05 '20 at 15:54
Can't directly return a Matrix, since I'm using opaque pointers. Even doing something like Matrix* m = &newMatrix(3, 5); outside of the module where struct Matrix is defined wouldn't be possible ("invalid use of incomplete typedef"). Thank you anyway for your suggestions. What I ended up with in the end is this: https://i.imgur.com/1mmPOzS.png And thanks for pointing out my oversight in the pointer arithmetic, by the way :) — Gian, Mar 05 '20 at 16:34

Function call makes const attribute of a struct change its value

2 Answers2