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

Don't allow copying file into its own path #90069

Merged

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Mar 31, 2024

When you copy file into its source path, it will truncate it and then fail to write, resulting in empty file. This PR adds a check to prevent that.

@KoBeWi KoBeWi added this to the 4.3 milestone Mar 31, 2024
@KoBeWi KoBeWi requested a review from a team as a code owner March 31, 2024 11:54
@AThousandShips
Copy link
Member

I'd suggest a more broad improvement here:

  • Offload copying inside the file system proper to platforms, using more efficient low level methods there
  • Use this just for copying between access types, like res:// -> user://, etc.

@KoBeWi
Copy link
Member Author

KoBeWi commented Mar 31, 2024

Offload copying inside the file system proper to platforms, using more efficient low level methods there

Like which one? The only difference in the OS-specific methods is that they always operate on absolute paths. And copy() is not even system-specific, it has general implementation. I'm not sure how to solve it more reliably, other than writing a method that untangles absolute paths.

Use this just for copying between access types, like res:// -> user://, etc.

??

@AThousandShips
Copy link
Member

I mean that this method is naïve and inefficient, and that unless we can't use a native method we might be better off using a native one, only if we're copying from inside a pck or exported project, or from memory, or network, it'd be useful to use native methods

But it does use absolute paths though, underlyingly, just in FileAccess, otherwise they couldn't do anything either, so id say that the solution here is both to improve performance, and solve this issue properly, by letting the platform handle it

@KoBeWi
Copy link
Member Author

KoBeWi commented Mar 31, 2024

The performance is not a concern here and I don't know any native method to handle that.

@AThousandShips
Copy link
Member

There should be generally, absolute paths aren't an issue as such here, or we couldn't do any file management, but anyway, I don't see this very specific check as a very useful safeguard generally

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this prevents truncating files, I'd say it's a good improvement already, even if the UX or getting an error message like this isn't great.

@akien-mga akien-mga merged commit c53a4a7 into godotengine:master Jun 21, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@KoBeWi KoBeWi deleted the file_suicide_prevention_measures branch June 21, 2024 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants