nibblebits / PeachOS

Simple kernel designed for a online course
GNU General Public License v2.0
133 stars 55 forks source link

process_map_elf: elf phdr's with filesize 0 cause incorrect mapping #26

Closed longoj closed 12 months ago

longoj commented 12 months ago

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!

nibblebits commented 12 months ago

Hi, I am glad you like the course and I am here for any questions you have. I really recommend you follow the course from start to finish rather than to go off on your own. I understand the feeling, it can get very exciting sometimes however unless your following the course exactly its possible you will make mistakes that were not present in the original content. I do encourage experimentation with what you have learned as of course it extends your knowledge further but it is better in a seperate copy of the project and theirs nothing wrong with mistakes of course thats how everyone learns.

So we can determine if this is a bug in the course to be fixed or a mistake you made with your version can you please clone the repository from the most recent commit and confirm if the strtok function is not functionable.

I also want to state mistakes have been made at various parts of the course but I fixed them in later lectures so any proposed bugs have to be checked against the master repository most recent commit to be sure that they are present

Thanks Dan

longoj commented 12 months ago

Hi Dan,

All understood. I don't view this as a bug/mistake with respect to the course. Even if this is not addressed, it is useful for others to know about it. As such, I will close the issue.

As stated above, this can be reproduced with the latest code on the master branch by simply adding the one line of code.

Thanks, James

nibblebits commented 12 months ago

Its possible its a mistake with how global variable data is stored, thanks for letting me know and hopefully it helps someone