Use allocation wrappers instead of direct call libc memory allocation

[mingodad]

I've noticed that there is direct call for memory allocation in several places and there is no check for a non NULL return value.

Probably it's a good idea to create wrappers for then and check for NULL return value there and print an error message and call exit when there is no more memory.

[thradams]

On Thu, 22 Feb 2024 at 12:32, Domingo Alvarez Duarte < @.***> wrote:

I've noticed that there is direct call for memory allocation in several places and there is no check for a non NULL return value.

Probably it's a good idea to create wrappers for then and check for NULL return value there and print an error message and call exit when there is no more memory.

The reason I postponed this is because on windows we can use a version of malloc that can detect leaks and show the error. It is also possible to adapt a macro xmalloc for instance, but to keep the leak report correct it must be implemented similarly of the original windows malloc implementation. the experimental -nullcheck will complain a lot of non checked pointer... this is not be used yet. it is modeled similar of c# nullchecks.

[thradams]

the other reason to keep calloc malloc is because GCC and cake will have a built in semantics. creating a wrapper removes at the caller side the assumptions the compiler does that malloc returns a initialized memory and calloc returns a zeroed memory.

[mingodad]

Looking into struct flow_defer_scope* flow_visit_ctx_push_tail_block(struct flow_visit_ctx* ctx) I noticed a pattern that you have that's use the return value that can be null and then check for it later (see comments //!! <<).

struct flow_defer_scope* flow_visit_ctx_push_tail_block(struct flow_visit_ctx* ctx)
{
    struct flow_defer_scope* owner p_block = calloc(1, sizeof * p_block);
    p_block->previous = ctx->tail_block; //!! << p_block can be NULL segfault here
    ctx->tail_block = p_block;
    return ctx->tail_block; 
}

void flow_visit_function(struct flow_visit_ctx* ctx, struct declaration* p_declaration)
{
    assert(p_declaration->function_body);

    assert(ctx->tail_block == NULL);
    struct flow_defer_scope* p_defer = flow_visit_ctx_push_tail_block(ctx);
    if (p_defer == NULL) //!!<< Checking for NULL but inside flow_visit_ctx_push_tail_block you don't check
    {
        return;
    }
    p_defer->p_function_body = p_declaration->function_body;

    flow_visit_declaration(ctx, p_declaration);

    if (!flow_is_last_item_return(p_declaration->function_body))
    {
        check_defer_and_variables(ctx, p_defer, p_declaration->function_body->last_token);
    }

    flow_visit_ctx_pop_tail_block(ctx);
}

[mingodad]

Also your style of naming structures and functions with the same name can be confusing and make things a bit harder for searching for then:

struct **generic_association* owner generic_association*(struct parser_ctx ctx)

[thradams]

When I need I search for struct generic_association. (I don't use typedefs)

[mingodad]

Also free first and use after:

            int code = type_common(&new_expression->left->type, &new_expression->right->type, &new_expression->type);
            if (code != 0)
            {
                expression_delete(new_expression); //!!<<  FREE first
                type_print(&new_expression->left->type); //!!<< USE AFTER FREE
                type_print(&new_expression->right->type); //!!<< USE AFTER FREE
                compiler_diagnostic_message(C_ERROR_INVALID_TYPE, ctx, ctx->current, "invalid types logicl and expression");
                throw;
            }
            p_expression_node = new_expression;

[thradams]

I was reviewing the use after free problem last night. Each solution has a drawback.

One detail is that I may have to add some "view pointer" let's say for a variable or function at symbol table. (The idea is that the symbol table is always pointing for something at AST that is alive, and lives longer than the symbol table)

But if for some reason the function has fatal errors, and I delete the function, then the symbol table is pointing to a deleted pointer.

If I don't delete it can be a memory leak. (Having it at symbol table but not at AST)

Still thinking about it...many alternatives but search the easiest one. One alternative is once I have a fatal error, I don't touch symbol table anymore.

[mingodad]

And the flow/owner analysis doesn't catch these easy ones (shown above) ?

[thradams]

not yet.

[thradams]

This one, current flow analysis must detect otherwise is a bug. After expression_delete(new_expression); //!!<< FREE first the static state of new_expression is uninitialized.

Then... type_print(&new_expression->left->type); //!!<< USE AFTER FREE is using a uninitialized variable.

            if (code != 0)
            {
                expression_delete(new_expression); //!!<<  FREE first
                type_print(&new_expression->left->type); //!!<< USE AFTER FREE
           }  

I will do a small sample.

[thradams]

struct Y{
  int dummy;
};

struct X{
  struct Y * _Owner pY;
};
struct X * _Owner newX();
void deleteX(struct X* _Owner p);
void f(struct Y* p);

int main(){
  struct X * _Owner p = newX();
  deleteX(p);
  f(p->pY);
}

c:/main.c:16:6: error: uninitialized object 'p.pY' 16 | f(p->pY); | ^~

http://thradams.com/cake/playground.html?code=DQpzdHJ1Y3QgWXsNCiAgaW50IGR1bW15Ow0KfTsNCg0Kc3RydWN0IFh7DQogIHN0cnVjdCBZICogX093bmVyIHBZOw0KfTsNCnN0cnVjdCBYICogX093bmVyIG5ld1goKTsNCnZvaWQgZGVsZXRlWChzdHJ1Y3QgWCogX093bmVyIHApOw0Kdm9pZCBmKHN0cnVjdCBZKiBwKTsNCg0KaW50IG1haW4oKXsNCiAgc3RydWN0IFggKiBfT3duZXIgcCA9IG5ld1goKTsNCiAgZGVsZXRlWChwKTsNCiAgZihwLT5wWSk7DQp9&to=-1&options=

[thradams]

ok found the problem..

struct Z{
  int dummy;
};

struct Y{
  int dummy;
  struct Z * _Owner pZ;
};

struct X{
  struct Y * _Owner pY;
};
struct X * _Owner newX();
void deleteX(struct X* _Owner p);
void f(struct Z* p);

int main(){
  struct X * _Owner p = newX();
  deleteX(p);
  f(p->pY->pZ);
}

I opened an issue.

owership failing after the 3 pointed object

126

I think it is easy to fix, just check for uninitialized at -> operator. Then at pY-> we have an error.