Ported C source, assigning pointer breaks struct, what did I do wrong?

Cursorkeys

I have some code that is, supposedly, working 'portable C'. My platform doesn't have dynamic allocation, so I'm having to adjust the code and statically allocate things.

Everything is going well apart from one bit that deals with a callback system. When the the callback structure is assigned into its parent structure all hell breaks loose, my debugger reports the parent struct suffers memory corruption.
I'm currently at a loss how it breaks the enclosing struct.

Before assigning callbacks:

After:

I've cut it down to what I think is the minimum involved code. Any suggestions gladly received!

modem_instance.c

static void handle_response(const char *buf, size_t len, void *arg)
{
    ...
}

static void handle_unrequested(const char *buf, size_t len, void *arg)
{
    ...
}

enum modem_response_type scan_line(const char *line, size_t len, void *arg)
{
    ...
}

static const struct modem_parser_callbacks parser_callbacks = {
    .handle_response = handle_response,
    .handle_unrequested = handle_unrequested,
    .scan_line = scan_line,
};

struct as_engine_t *as_instance_initialise(const char port, port_write platformWriteRoutine, port_read platformReadRoutine)
{
    ....
    //Reset the buffer parser
    modem->modem_command_engine.parser = modem_parser_initialise(&parser_callbacks, 256, (void *) modem);
    ....
}

modem_buffer.c

struct as_parser_t 
{
    const struct modem_parser_callbacks *callbacks;
    void *parent;

    enum as_parser_state state;
    bool expect_data;
    size_t data_left;
    int nibble;

    size_t *buffer;
    unsigned int buffer_used;
    unsigned int buffer_size;
    unsigned int buffer_current;
};

struct as_parser_t parser;
char static_buffer[PARSER_STATIC_BUF_SIZE];

struct as_parser_t *modem_parser_initialise(const struct modem_parser_callbacks *callbacks, size_t buffersize, void *parent)
{
    struct as_parser_t *parser = (struct as_parser_t *) &parser;
    
    //Set response buffer.
    parser->buffer = static_buffer;
    
    parser->callbacks = callbacks;   //<---------- PROBLEM IS HERE!
    parser->buffer_size = buffersize;
    parser->parent = parent;

    return parser;
}

modem_buffer.h

typedef enum modem_response_type (*modem_line_scanner_t)(const char *line, size_t len, void *parent);

typedef void (*modem_response_handler_t)(const char *line, size_t len, void *parent);

struct modem_parser_callbacks 
{
    modem_line_scanner_t scan_line;
    modem_response_handler_t handle_response;
    modem_response_handler_t handle_unrequested;
};

struct as_parser_t *modem_parser_initialise(const struct modem_parser_callbacks *callbacks, size_t buffersize, void *parent);

aitap

@Cursorkeys said in Ported C source, assigning pointer breaks struct, what did I do wrong?:

struct as_parser_t *parser = (struct as_parser_t *) &parser;

There should be no need to cast this pointer if the types match. If the compiler warns about this line, there might be a mistake elsewhere.

struct as_parser_t
{
size_t *buffer;

struct as_parser_t parser;

char static_buffer[PARSER_STATIC_BUF_SIZE];

parser->buffer = static_buffer;

You might want to change the pointer type to char * buffer.

But these are all pretty minor. I don't know what could cause the behaviour you observe.

Medinoc

@Cursorkeys Of course, you've already checked that the code is mono-thread and optimization is turned off on the compiler?

There's something weird with this line too:
struct as_parser_t *parser = (struct as_parser_t *) &parser;
Why is this cast necessary? It shouldn't be. Remove it, see if if stops compiling, study why.

Edit: Hanzo'd by aitap.

The more I look at this, the more I think this line actually initializes parser as a pointer to itself, leading to potential stack smashing and all hell breaking loose.

dkf

Just checking: you're not placing the callbacks structure on the stack, are you?

martijntje

@dkf Isn't that all he has? He doesn't have dynamic allocation. From the code he posted it seems to be static so that should be fine.

Cursorkeys

@aitap said in Ported C source, assigning pointer breaks struct, what did I do wrong?:

You might want to change the pointer type to char * buffer.

Done, thanks.

@aitap said in Ported C source, assigning pointer breaks struct, what did I do wrong?:

struct as_parser_t *parser = (struct as_parser_t *) &parser;

There should be no need to cast this pointer if the types match. If the compiler warns about this line, there might be a mistake elsewhere.

@Medinoc said in Ported C source, assigning pointer breaks struct, what did I do wrong?:

@Cursorkeys Of course, you've already checked that the code is mono-thread and optimization is turned off on the compiler?
There's something weird with this line too:
struct as_parser_t *parser = (struct as_parser_t *) &parser;
Why is this cast necessary? It shouldn't be. Remove it, see if if stops compiling, study why.
Edit: Hanzo'd by aitap.
The more I look at this, the more I think this line actually initializes parser as a pointer to itself, leading to stack smashing and all hell breaking loose.

Yep, no threading available and all optimisation turned off.

It does not like removing the cast so maybe that's a smoking gun:

ModemStack/src/modem_buffer.c: In function 'modem_parser_initialise': ModemStack/src/modem_buffer.c:42:34: warning: initialization from incompatible pointer type

dcon

@Medinoc said in Ported C source, assigning pointer breaks struct, what did I do wrong?:

The more I look at this, the more I think this line actually initializes parser as a pointer to itself, leading to potential stack smashing and all hell breaking loose.

Yes, that looks highly suspicious... Try changing one of those variable names.

PleegWat

@Medinoc said in Ported C source, assigning pointer breaks struct, what did I do wrong?:

The more I look at this, the more I think this line actually initializes parser as a pointer to itself, leading to potential stack smashing and all hell breaking loose.

That'll be it. The local pointer is shadowing the global struct. Why do you need a pointer anyway if it's always pointing to the same global struct?

Cursorkeys

@dcon said in Ported C source, assigning pointer breaks struct, what did I do wrong?:

@Medinoc said in Ported C source, assigning pointer breaks struct, what did I do wrong?:

The more I look at this, the more I think this line actually initializes parser as a pointer to itself, leading to potential stack smashing and all hell breaking loose.

Yes, that looks highly suspicious... Try changing one of those variable names.

That was exactly what it was. Thank you so much guys!

Maybe that'll teach me to to ask why the compiler is unhappy and not just cast. I'm still surprised you can actually do this though (assign the variable you're declaring to itself in its own initialisation).

@PleegWat said in Ported C source, assigning pointer breaks struct, what did I do wrong?:

That'll be it. The local pointer is shadowing the global struct. Why do you need a pointer anyway if it's always pointing to the same global struct?

I was trying to change as little as possible until I got the code at least working, so I could commit that and then start refactoring in earnest. It originally malloc'd the struct and used the pointer to that, so I wanted to keep the pointer for now.

dcon

@Cursorkeys Not sure about C, but in C++ there are warnings that can be enabled for variables being shadowed.

Cursorkeys

@dcon said in Ported C source, assigning pointer breaks struct, what did I do wrong?:

@Cursorkeys Not sure about C, but in C++ there are warnings that can be enabled for variables being shadowed.

Nothing listed in the compiler manual, but if I add -Wshadow it works just fine. Thanks, I'm definitely leaving that enabled.

Medinoc

@Cursorkeys said in Ported C source, assigning pointer breaks struct, what did I do wrong?:

Maybe that'll teach me to to ask why the compiler is unhappy and not just cast

Hence my sigquote on another forum:

"Aw, come on, who would be so stupid as to insert a cast to make an error go away without actually fixing the error?"
Apparently everyone.
-- Raymond Chen.