Memory parts are overlapping

Question about which tools to use, bugs, the best way to implement a function, etc should go here. Don't forget to see if your question is answered in the wiki first! When in doubt post here.
Post Reply
User avatar
osdever
Member
Member
Posts: 492
Joined: Fri Apr 03, 2015 9:41 am
Contact:

Memory parts are overlapping

Post by osdever »

Some days ago I'm realized that the memory parts are overlapping. So any call can wreck up all data. There's an example:
If I add 1 small printf, something will mess up. For example, without that call my WIP font drawing engine is working normal, except for some drawing errors:
Screenshot from 2016-01-16 19-54-46.png
Screenshot from 2016-01-16 19-54-46.png (1.71 KiB) Viewed 1716 times
But if I call it, it'll mess all up:
Screenshot from 2016-01-16 19-55-55.png
Screenshot from 2016-01-16 19-55-55.png (1.12 KiB) Viewed 1716 times
This is my memory.c:

Code: Select all

//Memory management.
#include "../include/memory.h"

#include <stddef.h>
#include <stdbool.h>
#include <stdint.h>
#include "../include/fio.h"

#define isBitSet(var, n) ((var & (1 << n)))

// Struct page is only for internal usage and should not be used anywhere else
#define PAGE_SIZE 4096
#define PAGE_MAX 4092 // (PAGE_SIZE - 2 * sizeof(uint16_t))
typedef struct
{
    uint16_t free_bytes;
    uint16_t free_offset;
    uint8_t  memory[PAGE_MAX];
} page;

size_t page_count;
page*  pages;

void* memcpy(void* dst, const void* src, size_t n)
{
    size_t i;
    if((uint8_t*)(dst) < (uint8_t*)(src))
    {
        for(i = 0; i < n; ++i)
        {
            ((uint8_t*)(dst))[i] = ((uint8_t*)(src))[i];
        }
    }
    else
    {
        for(i = 0; i < n; ++i)
        {
            ((uint8_t*)(dst))[n - i - 1] = ((uint8_t*)(src))[n - i - 1];
        }
    }
    return dst;
}

void* memset(void* ptr, uint8_t val, size_t n)
{
    size_t i;
    for(i = 0; i < n; ++i)
    {
        ((uint8_t*)(ptr))[i] = val;
    }
    return ptr;
}

void* memmove(void* dst, void* src, size_t n)
{
    if(dst != src)
    {
        memcpy(dst, src, n);
        free(src);
    }
    return dst;
}

/*
 * malloc function.
 * Allocating num bytes of memory and returning pointer to it.
 * Memory is located at the end of the kernel memory.
*/
extern uint16_t kernel_heap_start;
void* malloc(size_t num)
{
    size_t i;
    if(num < PAGE_MAX)
    {
        for(i = 0; i < page_count; ++i)
        {
            if(pages[i].free_bytes >= num + sizeof(uint16_t))
            {
                void* free_space = (void*)((size_t)(pages + i) + pages[i].free_offset);
                *((uint16_t*)(free_space)) = num;
                pages[i].free_bytes  -= num + sizeof(uint16_t);
                pages[i].free_offset += num + sizeof(uint16_t);
                return (void*)((uint16_t*)(free_space) + 1);
            }
        }
        perr("malloc: no free pages left");
        return 0;
    }
    else
    {
        perr("malloc: more than one page requested");
        return 0;
    }
}

void* calloc(size_t num, size_t size)
{
    size *= num;
    void* result = malloc(size);
    memset(result, 0, size);
    return result;
}

void *realloc(void* memblock, size_t size)
{
    if(memblock < (void*)(pages))
    {
        return malloc(size);
    }
    else
    {
        uint16_t oldsize = *((uint16_t*)(memblock) - 1);
        void* newmem = malloc(size);
        memmove(newmem, memblock, oldsize);
    }
    return memblock;
}

void free(void* ptr)
{
    uint16_t size = *((uint16_t*)(ptr) - 1);
    page* pg = (page*)((size_t)(ptr) - (size_t)(ptr) % PAGE_SIZE);
    pg->free_bytes -= size;
    if(pg->free_offset == (uint16_t)((size_t)(ptr) - (size_t)(pg) + size))
    {
        pg->free_offset -= size;
    }
}

uint8_t init_page(page* this)
{
    this->free_bytes = PAGE_MAX;
    this->free_offset = 2;
    if(*(uint16_t*)(this) != PAGE_MAX || *((uint16_t*)(this) + 1) != 2)
    {
        return 0; // false
    }
    return 1; // true
}


size_t init_memory()
{
    size_t pgptr = (size_t)&kernel_heap_start;
    pgptr += (PAGE_SIZE - pgptr % PAGE_SIZE);

    page_count = 0;
    pages = (page*)(pgptr);
    while(init_page(pages + page_count) && page_count < 2048) ++page_count;
    return page_count;
}

Developing U365.
Source:
only testing: http://gitlab.com/bps-projs/U365/tree/testing

OSDev newbies can copy any code from my repositories, just leave a notice that this code was written by U365 development team, not by you.
User avatar
Combuster
Member
Member
Posts: 9301
Joined: Wed Oct 18, 2006 3:45 am
Libera.chat IRC: [com]buster
Location: On the balcony, where I can actually keep 1½m distance
Contact:

Re: Memory parts are overlapping

Post by Combuster »

There's a method called unit testing. You can even use that to test memory management from userspace - just grab yourself some scratch memory and play with it. For now, I renamed malloc/free to my_malloc/my_free so that I can use them in conjunction and dump some stats about memory management.

Ergo, I tried to strategically cover a few usage options with the following:

Code: Select all

void main(void)
{
    // this replaces the start of init_memory so that it groks our test memory addresses
    void * memory = malloc(2049*4096);
    uintptr_t memaddr = (intptr_t) memory;
    memaddr = (memaddr + 0xFFF) & ~0xFFF;
    pages = (page *) memaddr;
    page_count = 0;
    // initialize the data structures
    size_t result = init_memory();

    printf("mem at 0x%lx, aligned 0x%lx, pages 0x%lx, result 0x%lx\n", (long)memory, (long)pages, page_count, result);

    printf("start: %d %d\n", (int)(pages[0].free_bytes), (int)(pages[0].free_offset));

    void * p1 = my_malloc(16);
    printf("alloc #1@16b: 0x%lx-0x%lx (%d %d)\n", (long)p1 & 0xffff, ((long)p1 & 0xffff) + 15, (int)(pages[0].free_bytes), (int)(pages[0].free_offset));

    void * p2 = my_malloc(16);
    printf("alloc #2@16b: 0x%lx-0x%lx (%d %d)\n", (long)p2 & 0xffff, ((long)p2 & 0xffff) + 15, (int)(pages[0].free_bytes), (int)(pages[0].free_offset));

	my_free(p1);
    printf("free #1: %d %d\n", (int)(pages[0].free_bytes), (int)(pages[0].free_offset));

    void * p3 = my_malloc(32);
    printf("alloc #3@32b: 0x%lx-0x%lx (%d %d)\n", (long)p3 & 0xffff, ((long)p3 & 0xffff) + 31, (int)(pages[0].free_bytes), (int)(pages[0].free_offset));

	my_free(p3);
    printf("free #3: %d %d\n", (int)(pages[0].free_bytes), (int)(pages[0].free_offset));

	my_free(p2);
    printf("free #2: %d %d\n", (int)(pages[0].free_bytes), (int)(pages[0].free_offset));
}
This gets me the following. A real unit test would actually test the values and fail with a message, but finishing it properly is left as an exercise. Instead some hand-written comments:
mem at 0x7f6d01fa6010, aligned 0x7f6d01fa7000, pages 0x800, result 0x800
start: 4092 2 [should be 4092 4, the preamble is 4 bytes]
alloc #1@16b: 0x7004-0x7013 (4074 34) [expected: 4074 20, a difference of 16+2 bytes]
alloc #2@16b: 0x7024-0x7033 (4056 52)
free #1: 4004 52 [expected: 4056 52, same as after alloc #2]
alloc #3@32b: 0x7036-0x7055 (3970 86)
free #3: 3938 54 [expected: 4056 52, same as after alloc #2]
free #2: 3922 54 [expected: 4074 34, same as after alloc #1]
Since I can't even seem to allocate the first piece of memory properly, I stopped looking for your overlap issue, but it might magically vanish by the time you sort these base cases out.
"Certainly avoid yourself. He is a newbie and might not realize it. You'll hate his code deeply a few years down the road." - Sortie
[ My OS ] [ VDisk/SFS ]
alexfru
Member
Member
Posts: 1112
Joined: Tue Mar 04, 2014 5:27 am

Re: Memory parts are overlapping

Post by alexfru »

JFYI.
catnikita255 wrote:

Code: Select all

#define isBitSet(var, n) ((var & (1 << n)))
This will only work for 0 <= n < 30, not for n >= 31. Because 1 is a signed int and shifting it into position 31 causes signed integer overflow and undefined behavior. At the very least use 1u instead of plain 1.
catnikita255 wrote:

Code: Select all

void* memcpy(void* dst, const void* src, size_t n)
{
    size_t i;
    if((uint8_t*)(dst) < (uint8_t*)(src))
    ...
}
You can't use relational operators to compare arbitrary pointers. It can cause undefined behavior.
catnikita255 wrote:

Code: Select all

void* memmove(void* dst, void* src, size_t n)
{
    if(dst != src)
    {
        memcpy(dst, src, n);
        free(src);
    }
    return dst;
}
The standard memmove() does not free any memory.
catnikita255 wrote:

Code: Select all

void* calloc(size_t num, size_t size)
{
    size *= num;
    void* result = malloc(size);
    memset(result, 0, size);
    return result;
}
What if malloc() returns NULL?
catnikita255 wrote:

Code: Select all

void *realloc(void* memblock, size_t size)
{
    if(memblock < (void*)(pages))
    {
        return malloc(size);
    }
    else
    {
        uint16_t oldsize = *((uint16_t*)(memblock) - 1);
        void* newmem = malloc(size);
        memmove(newmem, memblock, oldsize);
    }
    return memblock;
}
Relational operators again. Also using non-standard GNU extensions (pointers to void should not be used in pointer arithmetic and comparison other than for equality/inequality without first being cast to pointers to char).

Check realloc() documentation. There's special behavior with NULL pointers.

Also consider size 0. See the documentation for malloc() and realloc().
Post Reply