Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

diskrm command causes segmentation fault and double free in fox32os 0.3.0 #28

Open
Andpuv opened this issue Feb 19, 2025 · 0 comments
Open

Comments

@Andpuv
Copy link

Andpuv commented Feb 19, 2025

Hello,

First of all, congratulations on the project — it's really fascinating and well-designed!

While experimenting with the system, I encountered some emulator crashes when using the diskrm command in the shell provided with fox32os 0.3.0. Specifically:

  • Attempting to remove a disk that was never mounted results in a segmentation fault.
  • Attempting to remove an already removed disk causes a double free error, leading to an emulator abort.

I believe the issue originates from the following function:

void remove_disk(size_t id) {
    if (id > 3) { puts("attempting to access disk with ID > 3"); return; }
    printf("unmounting disk ID %d\n", (int) id);
    /* >>> Abort! <<< */
    fclose(disk_controller.disks[id].file);
    disk_controller.disks[id].size = 0;
}

The problem seems to stem from the lack of validation on disk_controller.disks[id].file. If the pointer is NULL, calling fclose results in undefined behavior. Additionally, once the file is closed, the pointer is not set to NULL, making a second call to remove_disk(id) cause a double free.

A potential fix could be:

void remove_disk(size_t id) {
    if (id > 3) { puts("attempting to access disk with ID > 3"); return; }
    printf("unmounting disk ID %d\n", (int) id);

    /* Prevent segmentation fault by ensuring the file pointer is valid */
    if (disk_controller.disks[id].file) {
        fclose(disk_controller.disks[id].file);
        disk_controller.disks[id].size = 0;

        /* Prevent double free by setting the pointer to NULL */
        disk_controller.disks[id].file = NULL;
    }
}

Additionally, if I understand correctly, disk 0 is the one from which the operating system boots. It might be a good idea to prevent its removal entirely, possibly by adding a check inside the OS shell command (diskrm.asm). A warning message could notify users that disk 0 cannot be removed.

Let me know what you think! Thanks for your hard work on this project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant