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

REPLACEFILE buffer overflow #17

Closed
sicklittlemonkey opened this issue Apr 1, 2018 · 2 comments · Fixed by sicklittlemonkey/cadius#1
Closed

REPLACEFILE buffer overflow #17

sicklittlemonkey opened this issue Apr 1, 2018 · 2 comments · Fixed by sicklittlemonkey/cadius#1

Comments

@sicklittlemonkey
Copy link
Contributor

This is why strdup() is considered harmful!

      char *prodos_file_name = strdup(param->prodos_folder_path);
...
        strcat(prodos_file_name, "/");
      strcat(prodos_file_name, file_name);

Would submit a PR but not sure how you want to handle it.
Other code in the project is very conservative with filename/path lengths, eg:

   char prodos_folder_path[1024];

Also worth keeping comments up to date - should be REPLACEFILE:

  /** ADDFILE <2mg_image_path> <target_folder_path> <file_path> **/
  if(!my_stricmp(argv[1],"REPLACEFILE") && argc == 5)
    {
      param->action = ACTION_REPLACE_FILE;

Cheers,
Nick.

@mach-kernel
Copy link
Owner

Good find! I'd be happy to receive a PR if you have it or if you'd like I can fix it with the other bug that was opened. 😸

@sicklittlemonkey
Copy link
Contributor Author

I don't have a PR ready, and my allotted retro time for today is done!

It took me longer than I'd like to admit to find this. In a VS debug build the error would appear on cleanup in free()ing code, so I spent a couple of hours understanding the code and tracking back - until I realized it wasn't a free problem at all, and the likeliest place for a buffer overflow was the latest changes.

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

Successfully merging a pull request may close this issue.

2 participants