nibblebits / peachos Goto Github PK

Hey Dan!

I found a memory leak in the fopen() function.
In case of an error, we do not free root_path or descriptor_private_data. this would create a memory leak, especially if the error occurs on line 175 (PeachOS-master). My proposed solution is as follows:

out:
// fopen shouldnt return negative values
if (res < 0)
{
if(root_path)
pathparser_free(root_path);
if(descriptor_private_data)
kfree(descriptor_private_data);
res = 0;
}
return res;

In boot.asm:

ES segment register is set to zero on line 41,

At the end of procedure ata_lba_read on line 145

; We need to read 256 words at a time
      mov ecx, 256
      mov dx, 0x1F0
      rep insw
      pop ecx
      loop .next_sector
      ; End of reading sectors into memory
      ret

Since ES is zero, instruction rep insw relies on a ES segment that points to the NULL entry of the GDT table, this shouldn't work or could throw an exception, but works on qemu, maybe it's a bug of qemu or an undefined behaviour of the cpu.

To fix it, at label start32 that begins on line 85 we can set the ES register to the data segment in GDT table:

So :

[BITS 32]
 load32:
    mov eax, 1
    mov ecx, 100
    mov edi, 0x0100000
    call ata_lba_read
    jmp CODE_SEG:0x0100000

Can be changed to something like this:

[BITS 32]
 load32:
    mov ax,DATA_SEG
    mov es,ax
    mov eax, 1
    mov ecx, 100
    mov edi, 0x0100000
    call ata_lba_read
    jmp CODE_SEG:0x0100000

To whom it may concern.

I hope you are doing well and are in good health.

Thank you for the great course content, I have been thoroughly enjoying it.

I am currently busy with this video in section 4: "21. Loading our 32 bit kernel into memory and working with debugging symbols" .

Up until now I believe I have followed the course correctly, I have re-watched this video multiple times.

After setting the break point on "_start", and running the "target remote | qemu....." then pressing c, the code runs without breaking. Has anyone else had this problem?

I did load the symbols at address "0x0100000" using the "add-symbol-file"

command.

On boot.asm, you subtracted 1 from the size of the gdt. But not on the c version of it. Any reason for that?

ine 80 of src/boot/boot.asm:

gdt_descriptor:

dw gdt_end - gdt_start-1

dd gdt_start

line 134 of kernel.c:

gdt_load(gdt_real, sizeof(gdt_real));

and src/gdt/gdt.asm

[BITS 32]
section .asm
global gdt_load

gdt_load:
mov eax, [esp + 4]
mov [gdt_descriptor + 2], eax
mov ax, [esp + 8]
;should there be a
;sub ax, 1
mov [gdt_descriptor], ax
lgdt [gdt_descriptor]
ret

section .data
gdt_descriptor:
dw 0 ;size
dd 0 ;gdt start address.

aditya@Aditya:~/Desktop/PeachOS$ ./build.sh

i686-elf-gcc -I./src -I./src/string -g -ffreestanding -falign-jumps -falign-functions -falign-labels -falign-loops -fstrength-reduce -fomit-frame-pointer -finline-functions -Wno-unused-function -fno-builtin -Werror -Wno-unused-label -Wno-cpp -Wno-unused-parameter -nostdlib -nostartfiles -nodefaultlibs -Wall -O0 -Iinc -std=gnu99 -c ./src/string/string.c -o ./build/string/string.o
Assembler messages:
Fatal error: can't create ./build/string/string.o: No such file or directory
make: *** [Makefile:121: build/string/string.o] Error 1

In the lecture 94, you have elf32_off defined as a int32_t. The manual I am looking at describes it as an unsigned 4 byte integer. Wouldn't that be uint32_t?

I noticed a bug in my own PeachOS code which turned out to be to do with the order of registers in task.h struct registers. The order of cause matters because struct registers gets passed to assembly functions: task_return and restore_general_purpose_registers.

It follows if the order in memory matters then struct registers should be attribute((packed)) to ensure no compiler optimisations.

On file idt.h INTERRUPT_CALLBACK_FUNCTION should be
typedef void (*INTERRUPT_CALLBACK_FUNCTION)(struct interrupt_frame *frame);

Inside the method static int fat16_get_fat_entry(struct disk *disk, int cluster) in fat16.c,
there's the following line:
res = diskstreamer_seek(stream, fat_table_position * (cluster * PEACHOS_FAT16_FAT_ENTRY_SIZE));

However, and Daniel confirmed this as well, it should be
res = diskstreamer_seek(stream, fat_table_position + (cluster * PEACHOS_FAT16_FAT_ENTRY_SIZE));

The FAT table is just a contiguous area of memory, with each entry two bytes in size. So the nth entry in the table should be n * entry_size bytes from the beginning of the table. I.e. fat_table_position + (cluster * PEACHOS_FAT16_FAT_ENTRY_SIZE)

Hi Dan,

First, I very much enjoyed your course!

While following the course I began filling in functionality myself instead of following along exactly. After getting to the stdlib implementation portion I ran into an issue testing with strtok where the program does not fault, but strtok would not function correctly.

After investigating, I tracked it down to the elf image. There are two elf headers in BLANK.ELF that look like the following (for example):

    Program Headers:
      Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
      LOAD           0x001000 0x00400000 0x00400000 0x02008 0x02008 R E 0x1000
      LOAD           0x000000 0x00403000 0x00403000 **0x00000** 0x00010 RW  0x1000

As shown the FileSiz is 0. This means there are no bytes representing this header in memory when the elf file is read into memory. In effect, the global pointer variable used in strtok is mapped to whatever happens to be at that offset in memory. Since the memory is mapped to a valid area, there is no fault; however, the behavior of strtok is unpredictable depending on what exists in the file area that is mapped.

Steps to reproduce:

1. Add the following line just before the global variable declaration in stdlib/src/string.c as follows.
   ...
   char test[] = "THIS IS A TEST";
   char* sp = 0;
   char* strtok(char* str, const char* delimiters)
   {
   ...
   }

2. Change BLANK.ELF to process a string with strtok.

Result:
strtok always returns NULL

In the spirit of keeping things simple, a possible fix (not ideal or reliable) would be to modify process_map_elf and check if p_filesz != p_memsz. Then allocate the necessary memory (and copy the bytes from the file's memory if p_filesz is not zero).

This would prevent others from running into unexpected issues as they go through the course (in the event the elf file is laid out differently due to many factors).

Thanks for producing great content!

Hey Dan,

It seems that the current symbol file is giving incorrect memory addresses in GDB.

My assumption and correct me if I am wrong is that i686-elf-ld -g -relocatable $(FILES) -o ./build/kernelfull.o is using a default link file and does not add the correct memory offset that we assign in the linker file . = 1M;. Therefore kernelfull.o assumes that addresses start from 0. Doing nm ./build/kernelfull.o confirms this.

My solution was to create an exact copy of linker.ld, except that this time we change the OUTPUT_FORMAT to be elf32-i386 instead of binary.

This allows me to add another command during the ./bin/kernel.bin file which creates the correct symbol file.

Full version:

./bin/kernel.bin: $(FILES)
	i686-elf-ld -g -relocatable $(FILES) -o ./build/kernelfull.o
	i686-elf-gcc $(FLAGS) -T ./src/linker.ld -o ./bin/kernel.bin -ffreestanding -O0 -nostdlib ./build/kernelfull.o
	i686-elf-gcc $(FLAGS) -T ./src/linker-elf.ld -o ./build/kernelfull-elf.o -ffreestanding -O0 -nostdlib ./build/kernelfull.o

After the build instead of ./build/kernelfull.o I am using ./build/kernelfull-elf.o for the add-symbol-file command in gdb.

Here are a couple of videos explaining the situation in visual format.

Incorrect address: https://drive.google.com/file/d/1zoxyxB-XDsgWz5_OqoqxOIVgC8lmpLgA/view?usp=share_linkCorrect
Correct address: https://drive.google.com/file/d/1dd-qMhGv-rzCfYXR-U6lOTISHDJsLg1h/view?usp=share_linkNM Output: https://drive.google.com/file/d/1vakDElifvr5mGEVeEjtpXzXKkTOrahi9/view?usp=share_link

Let me know if further information is needed

I have a confusion about a function at heap.c.
Does following line at function heap_mark_blocks_taken() has "off by one" error?

if (i != end_block -1) { entry |= HEAP_BLOCK_HAS_NEXT; }

Should it be following instead:

if (i != end_block ) { entry |= HEAP_BLOCK_HAS_NEXT; }

/*

for example: if total_block =4 and start_blok =0, so end_block=3

so we want to mark(set) the block0, block1 and block2 with flag HAS_NEXT.

*/

Tried running in bochs emulator, and it seems to be a bit more strict than qemu, so it caught some issues.
Here are issues that I spotted after trying bochs:

1. Data segment registers aren't set in boot.asm

This causes a crash when trying to read kernel code to 0x100000, since default segments don't allow using addresses larger than 0xFFFF. Fixed it by moving step2 code under load32 instead and using DATA_SEG instead of 0x00:

load32:
    mov ax, DATA_SEG
    mov ds, ax
    mov es, ax
    mov ss, ax
    mov sp, 0x7c00

2. PIC interrupt remapping in kernel.asm missing ICW3 command

ICW1, ICW2, and ICW4 are currently sent, but ICW3 is skipped, which bochs doesn't like.
Fixed it just by copying code in the PIC page on OSDev (https://wiki.osdev.org/8259_PIC):

; Remap the master PIC
mov al, 00010001b ; ICW1: Start init sequence
out 0x20, al
mov al, 0x20 ; ICW2: Start at interrupt 0x20
out 0x21, al
mov al, 4 ; ICW3: Slave PIC IRQ line
out 0x21, al
mov al, 00000001b ; ICW4: Enable 8086 mode
out 0x21, al

3. tss struct is missing some fields

The ss1 and ssp fields aren't included in tss.h. These fields aren't used directly, but they affect the size of the struct, which is referenced in the GDT limit field. The error happens since bochs expects that size field to be at least 104 bytes.

Fixed by adding the 2 missing fields:

struct tss
{
    uint32_t link;
    uint32_t esp0;
    uint32_t ss0;
    uint32_t esp1;
+   uint32_t ss1;
    uint32_t esp2;
    uint32_t ss2;
    uint32_t cr3;
    uint32_t eip;
    uint32_t eflags;
    uint32_t eax;
    uint32_t ecx;
    uint32_t edx;
    uint32_t ebx;
    uint32_t esp;
    uint32_t ebp;
    uint32_t esi;
    uint32_t edi;
    uint32_t es;
    uint32_t cs;
    uint32_t ss;
    uint32_t ds;
    uint32_t fs;
    uint32_t gs;
    uint32_t ldtr;
    uint32_t iopb;
+   uint32_t ssp;
} __attribute__((packed);

In the function fat16_get_root_directory we first calculate the total_sectors (https://github.com/nibblebits/PeachOS/blob/master/src/fs/fat/fat16.c#L215-L219):

    int total_sectors = root_dir_size / disk->sector_size;
    if (root_dir_size % disk->sector_size) {
        total_sectors += 1;
    }

and then at the end of the function set the ending_sector_pos like so (https://github.com/nibblebits/PeachOS/blob/master/src/fs/fat/fat16.c#L246):

    directory->ending_sector_pos = root_dir_sector_pos + (root_dir_size / disk->sector_size);

This not only leaves total_sectors completely unused but recalculates the division which is inefficient and introduces a bug if I'm not mistaken. This was also outlined in this PR from last year: #7 .

This should be fixed by changing the assignment similar to what the PR suggests to(https://github.com/nibblebits/PeachOS/pull/7/files):

    directory->ending_sector_pos = root_dir_sector_pos + total_sectors;

Note that the - 1 is missing in comparison to the PR, otherwise fread doesn't seem to work.

I just bring this up, so it's not overlooked when revising the code for part 2 of the course and people see it's gonna be addressed as not everybody is going to check the closed PRs.

Correct me if I'm wrong but we are just ORing the high and low bits here:

Shouldn't we shift the high bits like so:

return (item->high_16_bits_first_cluster << 16) | item->low_16_bits_first_cluster;

Hey Deniel,

After the running a simmilar command to install qemu-system-x86_64 on an Ubuntu Virtual Box install on Windows I get;
'gtk initialization falied'

Please advise :) Googling on my own and will update;
Scott

When we call task_return function we for some reason push FLAGS which are from kernel code but not from the userspace code. This causes userspace code to have wrong FLAGS after context switching.

Here is a simple proof-of-concept for user space program blank.c (roginvs/udemy_kernel@a384f97):

    mov eax, 0
    cmp eax, 0
    .loop
    nop
    jz .loop
    nop
    ret

This looks like infinite loop. But when timer interrupt occurs and control is transferred back to userspace code then EFLAGS have ZF=0 and it causes this loop to break.

A solution is here https://github.com/nibblebits/PeachOS/pull/6/files - we have to push to stack EFLAGS the same way as other registers and also we have to provide initialization value for EFLAGS for userspace code.

On lines 559-566 of fat16.c:

if (item->attribute & FAT_FILE_SUBDIRECTORY)

{

    f_item->directory = fat16_load_fat_directory(disk, item);

    f_item->type = FAT_ITEM_TYPE_DIRECTORY;

}



f_item->type = FAT_ITEM_TYPE_FILE;

f_item->item = fat16_clone_directory_item(item, sizeof(struct fat_directory_item));

should this be:

if (item->attribute & FAT_FILE_SUBDIRECTORY)

{

    f_item->directory = fat16_load_fat_directory(disk, item);

    f_item->type = FAT_ITEM_TYPE_DIRECTORY;

} **else {**

f_item->type = FAT_ITEM_TYPE_FILE;

f_item->item = fat16_clone_directory_item(item, sizeof(struct fat_directory_item));

}

but an item is only either a file or a directory. On the original code, the last 2 lines in the snippet always gets executed, so everything is a file.

Hey!
There are a few problems with the code to switch to pmode:

1. we should turn on a20 line before switching to pmode
2. we set the segment registers in kernel.asm, but it should be done even before we call the data driver in boot.asm

sample code at https://serpaeos.sourceforge.io/

In src/keyboard/classic.c: line 92

task_page() is called.

When there is no task running, and the keyboard interrupt occurs, current_task is NULL/garbage.
CR3 is loaded with some garbage value causing unexpected behaviour.

Regards,
Yuvraj Sakshith

In lecture 94, Implementing the ELF Loader - Part 2, we create the funciton static bool elf_is_executable(struct elf_header* header) since we are only working with executable files, but later at the video, when creating the int elf_validate_loaded(struct elf_header* header) function, we do not call it to validate the file format.

Hi!
In PeachOS Master, there is a possible memory leak in process_load_for_slot.
Say we get an error on line 500:
`res = process_load_data(filename, _process);
if (res < 0)
{
goto out;
}

program_stack_ptr = kzalloc(PEACHOS_USER_PROGRAM_STACK_SIZE);
if (!program_stack_ptr) //line 500
{
    res = -ENOMEM;
    goto out;
}`

then process data needs to be freed. But this doesn't happen. As it stands, the out label acknowledges the need to free the data but doesn't do so:
`out:
if (ISERR(res))
{
if (_process && _process->task)
{
task_free(_process->task);
}

   // Free the process data
}`

A better implementation could be:
`if (ISERR(res))
{
if (_process && _process->task)
{
task_free(_process->task);
}

    // Free the process data
    process_free_program_data(_process);
}`

https://github.com/nibblebits/PeachOS/blob/master/src/fs/fat/fat16.c#L411:

if (entry == 0xFF8 || entry == 0xFFF)

This line is checking if the cluster entry is the last cluster in a file. However, the value it is checking for is incorrect. The EOC (end of chain) mark for FAT16 should be 0xFFF8 - 0xFFFF.

Hey Dan!
Correct me if i am wrong, but I believe there is an error in fat16_read. We never advance fat_desc->pos, thus in a situation such that we would read from a file multiple times, we would read and re-read the same data in the file. The following program when inserted into the function kernel_main will demonstrate the problem:
int fd = fopen("0:/hello.txt", "r"); for(int i = 0; i < 5; i++) { char buf [11]; fread(buf, 10, 1, fd); print(buf); } while(1);

my solution is to insert into fat16_read at line 716 in the PeachOS master:
fat_desc->pos = offset;

thanks!

Jaihson

`

elf_file->elf_memory = kzalloc(stat.filesize);
res = fread(elf_file->elf_memory, stat.filesize, 1, fd);
if (res < 0)
{
    goto out;
}

res = elf_process_loaded(elf_file);
if(res < 0)
{
    goto out;
}

`

in elfloader.c file, in the elf_load function, we dont free the memory we allocated with kzalloc, if we fail to read the file or if we fail to load the file

process_load_for_slot kzallocs a new struct process, but never kfrees it. Possible solution:
process_unlink():
processes[process->id] = 0;
kfree(process);
if (current_process == process)
{
process_switch_to_any();
}

We find the starting sector on line 540 (in PeachOS master), but we never set directory->starting_sector