Click here to Skip to main content
15,887,214 members
Please Sign up or sign in to vote.
0.00/5 (No votes)
See more:
can you all take a look at this linked list and tell me if you see anything wrong with it? I'm running into heap corruption. This is the only thing doing allocations explicitly in my code.

What I have tried:

C++
typedef class screen_info {
public:
    int index;
    canvas_t icon;
    label_t value;
    canvas_t bar;
    graph_buffer_t graph;
    screen_packet_t packet;
    screen_info* next;
    char value_sz[64];
    char old_format[16];
    float old_value_max;
    screen_info();
    screen_info(const screen_info& rhs);
    screen_info& operator=(const screen_info& rhs);
    virtual ~screen_info();
} screen_info_t;
...

screen_t main_screen;
screen_info_t* first_segment = nullptr;
screen_info_t* tail_segment = nullptr;

screen_info::screen_info() : index(-1),icon(main_screen),value(main_screen),bar(main_screen), graph(), packet(), next(nullptr) {

}
screen_info::screen_info(const screen_info& rhs) : index(rhs.index), icon(rhs.icon), value(rhs.value), bar(rhs.bar), graph(rhs.graph), packet(rhs.packet), next(rhs.next) {

}
screen_info& screen_info::operator=(const screen_info& rhs) {
    index = rhs.index;
    icon = rhs.icon;
    value = rhs.value;
    bar = rhs.bar;
    graph = rhs.graph;
    packet = rhs.packet;
    next = rhs.next;
    return *this;
}
screen_info::~screen_info() {
    icon.on_paint(nullptr);
    bar.on_paint(nullptr);
    graph.clear();
}

screen_info_t* segment_add() {
    auto result = new screen_info_t();
    if(first_segment==nullptr) {
        first_segment = result;
        tail_segment = result;
        return result;
    }
    if(result==nullptr) { return result; }
    tail_segment->next = result;
    tail_segment = result;
    return result;
}
size_t segment_count() {
    screen_info_t* seg=first_segment;
    size_t result = 0;
    while(seg!=nullptr) {
        seg=seg->next;
        ++result;
    }
    return result;
}
void segment_clear_all() {
    screen_info_t* seg=first_segment;
    while(seg!=nullptr) {
        screen_info_t* n = seg->next;
        delete seg;
        seg=n;
    }
    first_segment = nullptr;
    tail_segment = nullptr;
}
Posted
Updated 27-Jun-23 5:34am
v3
Comments
Mike Hankey 22-Jun-23 12:50pm    
The only thing I see and I'm pretty blind is to p[ut the check for nullptr after the new.
Other than that nothing sticks out,

screen_info_t* segment_add() {
auto result = new screen_info_t();
if(result==nullptr) { return result; }
if(first_segment==nullptr) {
first_segment = result;
tail_segment = result;
return result;
}

tail_segment->next = result;
tail_segment = result;
return result;
}
honey the codewitch 22-Jun-23 14:27pm    
thanks. yeah i could put that in to make it more explicit but it's essentially already checked by other if.
CPallini 22-Jun-23 14:09pm    
I see nothing wrong in your list implementation.
BernardIE5317 27-Jun-23 13:11pm    
link below may be useful also stl smart pointers also full running code for repro - cheerios
https://learn.microsoft.com/en-us/cpp/c-runtime-library/debug-versions-of-heap-allocation-functions?view=msvc-170

Be gentle, I’m still teaching myself C++.

Both your copy constructor and copy assignment operator are setting the “next” value: shouldn’t you make them nullptr like you did in the default constructor, then set them later when adding it into the linked list?
 
Share this answer
 
Comments
honey the codewitch 22-Jun-23 19:42pm    
It kind of depends on what you mean by "copy". Both ways are valid C++, but the kind of copy it is is a little different. If I copy a segment that's linked to segment B, then there will be two segments leading to segment B. That's an invalid link list of course, even though the copied instance is valid which owes to your concern, but the way this copy constructor is used that will never be an issue. Good eye though. :)
Hi,

IMHO your problem are at least assignment and copy constructor. It is possible to build a list, where the same memory block is used more than once in the list.

When you do a segment_clear_all(), the same memory block is deleted multiple times. So I guess your problem arises on segment_clear_all().

When you assign a screen_info, what happens with the data to which the old next pointer points? Doesn't it become a lost memory block which is never deleted?
 
Share this answer
 
Comments
honey the codewitch 26-Jun-23 20:03pm    
Those copy constructors are never getting invoked in an instance where it would create an invalid linked list. That much is tested code. So the delete code works correctly. Every next pointer created is valid until segment_clear_all() at which point none are.
Member 12995087 27-Jun-23 11:34am    
Do you have example code for screen_info_t usage which can reproduce the heap corruption?
You didn't say something if you do new/delete in IRQ or different tasks.

You are sure, it comes from screen_info_t and you call the class only from one task or from main loop?

Memory at address 0x00000000 is valid? iMXRT can be configured in this dangerous way...

When you have the possibility to reproduce the corruption, may be you can set a write breakpoint for the corrupted address and find out, which code part is writing to it.
honey the codewitch 27-Jun-23 11:42am    
This is the only place new or delete are being called, in the shown code. this is on ESP32s and SAMD51s, not ARMs. I don't have a debugger on these devices. Even JTAG is too slow to be usable. Besides, I'm not getting a stack dump. I'm getting a hang. And only on devices with a certain type of LCD display bus.
Member 12995087 28-Jun-23 4:23am    
May be you can print out via serial or whatever interface a history of Malloc/Free/New/Delete like M0x12345678 F0x12345678... to find out, if a double free/delete or a free/delete of a never allocated memory block is causing the problem?

However, there is always the chance that a bug somewhere else causes corruption in a completely other software module.

If debugging on the ESP32 is so hard, it may help to create a simple Windows project containing your code, where the functions accessing hardware are filled with dummy code delivering the expected function results. In this way, you can use the full power of Visual Studio to find out what is going wrong.

This content, along with any associated source code and files, is licensed under The Code Project Open License (CPOL)



CodeProject, 20 Bay Street, 11th Floor Toronto, Ontario, Canada M5J 2N8 +1 (416) 849-8900