nibblebits / PeachOS

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

Fix some bugs in fat16_read and fat16_seek #15

Closed twistdroach closed 1 year ago

twistdroach commented 1 year ago

Fix bug in fat16_seek where it was possible to seek beyond EOF when using SEEK_CUR

Fix a number of bugs in fat16_read:

The last one (about reading beyond EOF) I'm especially unsure of (I'm not a fat filesystem guru), so it's possible this should already be working and there's some bug deeper in the fat16 reading code I'm just hiding...

PS - course was great!

nibblebits commented 1 year ago

Hello, Thanks for this, unable to accept the pull request because this repository must follow lecture videos I.e lecture 1 , lecture 2 ect ect.

However I will be reviewing all of your commits and if they are valid I will include bug fixes in a future lecture.

Thanks for your assistance and I am very glad you are enjoying the course

Thanks Dan

nibblebits commented 1 year ago

Regarding the fat16_seek the check is already being done

    if (offset >= ritem->filesize)
    {
        res = -EIO;
        goto out;
    }

Seek line 734 https://github.com/nibblebits/PeachOS/blob/c39054f075aada561197c7f1d07418175d50bf54/src/fs/fat/fat16.c

Thanks again for reporting things to me, even when unsure it can be helpful as everyone makes mistakes including me

I will take a look at the other commits when i get time. I plan to fix a good couple bugs soon

twistdroach commented 1 year ago

That's only valid for SEEK_SET, SEEK_CUR adds the offset + current position..

    case SEEK_CUR:
        desc->pos += offset;
        break;
nibblebits commented 1 year ago

Hi,

Can you explain what you believe to be the mistake as I am not seeing the bug your talking about Thanks

twistdroach commented 1 year ago

Sorry just realized you replied. Problem is this - imagine the descriptor's pos is something larger than zero...we then are called with SEEK_CUR flag. We check if the offset is beyond the end of the file BUT for SEEK_CUR we need to check the (offset + the current position). Otherwise, it's possible to seek passed filesize, because we only checked offset not offset+pos.

nibblebits commented 1 year ago

Thanks I will verify next week what you have mentioned and if your correct I will create a new lecture to fix it. Have a great weekend