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

Fix Windows platform file access to allow file sharing with external programs #51430

Merged
merged 2 commits into from
Aug 10, 2021

Conversation

mhilbrunner
Copy link
Member

@mhilbrunner mhilbrunner commented Aug 9, 2021

This restores Windows platform file handling back to open files non-exlusively by default, as was the case before October 2018. (See b902a2f)

It seems back then it was unintentionally changed while fixing warnings.

This is fixed by changing the open call from _wfopen_s() to _wfsopen(), thus being able to set the file sharing constant to _SH_DENYNO while still preserving security features of _wfopen_s(), like parameter validation, meaning the warning stays fixed.

Fixes #28036.

I tested this change on Windows as extensively as I could and found no issues.


Details

Back then, while fixing warnings for MSVC, the function used for opening files was changed from _wfopen() to _wfopen_s() as suggested by the warning C4996. ("This function may be unsafe, consider using _wfopen_s instead.")

This new function

  1. did parameter validation and thus avoided some possible security issues due to nil pointers or wrongly terminated strings
  2. it also changed the default file sharing for opened files from _SH_DENYNO (which was the implicit default for the previous _wfopen()) to _SH_SECURE.

_SH_DENYNO means every opened file could be opened by other calls (like is the default on other operating systems).
_SH_SECURE means if the file is opened with READ access, others can still read the same file (but not write to it), but if it is opened with WRITE access, others can't open it at all, not even to read.

As this file sharing access is implied as default, not a parameter for either function, it seems this change was neither intended nor noticed.

This led to rarely occuring bugs on Windows, i.e. due to random access by Antivirus processes, or Godot/Windows not closing a file handle fast enough while trying to open it again elsewhere (i.e. project.godot, instead showing the Project Manager, or saving shaders/debugging the game).

What this PR does is change the file access to a third method, _wfsopen(). This is still secure, doing parameter validation and thus avoids the warning, but it allows us to actually SET the file sharing parameter. And we set it to _SH_DENYNO, as it was implicitely before the change. (And as it currently is on all non-Windows platforms, where file sharing restrictions don't exist by default.)

Warning C4996 should really have been pointing this out. It should've been _wfsopen() all along. Let's hope this banishes those annoying, rare errors for all eternity.

The only interesting lines are 116 and 316, changing the function used for opening in FileAccessWindows::_open() and FileAccessWindows::file_exists() respectively, everything else are formatting fixes.

Links:
_wfopen() docs
_wfopen_s() docs
_wfsopen() docs

@mhilbrunner mhilbrunner requested a review from a team as a code owner August 9, 2021 09:43
@mhilbrunner mhilbrunner added this to the 4.0 milestone Aug 9, 2021
@akien-mga akien-mga changed the title Fix Windows platform file access Fix Windows platform file access to allow file sharing with external programs Aug 9, 2021
@akien-mga akien-mga added cherrypick:3.3 cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Aug 9, 2021
@mhilbrunner
Copy link
Member Author

mhilbrunner commented Aug 9, 2021

Short explanation on sharing modes:

_SH_DENYNO: No restrictions. Multiple processes can open the file for reading, writing, or any combination thereof.
This was the default in Godot on Windows before 2018, is the default for vanilla fopen(), and is currently the case on all non-Windows platforms.
This PR changes all file access back to this.
This allows tail -f to read the file while it's being written to, but also allows e.g. multiple log writers to append to the same log file.

_SH_DENYRW: Denies other processes read and write access. Nothing allowed.

_SH_DENYRD: Denies other processes read access. They can still write.

_SH_DENYWR: Denies other processes write access. They can still read.

_SH_SECURE: The current default since 2018, before this PR. If the file is open for reading, other processes can read, but not write. If the file is open for writing, other processes can do nothing, not even read.
So if the file is open for writing, this is equivalent to _SH_DENYRW (deny all), otherwise to _SH_DENYWR (deny writing).

I debated instead changing it to _SH_DENYWR (deny writing for other processes), and in other applications would have, but that does prohibit some valid use cases (e.g. logging, as mentioned, or database uses), while I don't see any benefit for a game engine like Godot. Also, _SH_DENYNO is consistent with all other platforms, where Godot is already fine with all files being shared. Other FOSS projects also seem to favor _SH_DENYNO.

@RandomShaper
Copy link
Member

I'm not very fond of the presence of the cosmetic changes in the same commit, but looks good to me overall.

@mhilbrunner
Copy link
Member Author

I have no problem seperating those to a second commit if you want :) Just didn't bother because it was only 9 lines.

This restores Windows platform file handling back to open files non-exlusively by default, as was the case before October 2018. (See godotengine@b902a2f)
Back then, while fixing warnings for MSVC, the function used for opening files was changed from _wfopen() to _wfopen_s() as suggsted by the warning C4996. ("This function may be unsafe, consider using _wfopen_s instead.")

This new function
1. did parameter validation and thus avoided some possible security issues due to nil pointers or wrongly terminated strings
2. it also changed the default file sharing for opened files from _SH_DENYNO (which was the implicit default for the previous _wfopen()) to _SH_SECURE.

_SH_DENYNO means every opened file could be opened by other calls (like is the default on other operating systems).
_SH_SECURE means if the file is opened with READ access, others can still read the same file, but if it is opened with WRITE access, others can't open it at all, not even to read.

This led to rarely occuring bugs on Windows, i.e. due to random access by Antivirus processes, or Godot/Windows not closing a file handle fast enough while trying to open it again elsewhere (i.e. project.godot, instead showing the Project manager, or saving shaders/debugging the game).

What this PR does it change the file access to a third method, _wfsopen(). This is still secure, doing parameter validation and thus avoids the warning, but it allows us to actually SET the file sharing parameter. And we set it to _SH_DENYNO, as it was implicitely before the change. (And as it currently is on all non-Windows platforms, where file sharing restrictions don't exist by default.)

Warning C4996 should really have been pointing this out. It should've been _wfsopen() all along. Let's hope this banishes those annoying, rare errors for all eternity.

Fixes godotengine#28036.
@akien-mga
Copy link
Member

I have no problem seperating those to a second commit if you want :) Just didn't bother because it was only 9 lines.

I think it's OK in this case as it's indeed just a few lines. For more substantial changes keeping them separate is good practice indeed.

@mhilbrunner
Copy link
Member Author

Seperated cosmetic changes into a second commit, added a dot :)

@akien-mga akien-mga merged commit 4bfb49b into godotengine:master Aug 10, 2021
@akien-mga
Copy link
Member

Thanks!

@bruvzg
Copy link
Member

bruvzg commented Aug 10, 2021

MinGW (9.0.0, GCC 11.2.0) build on macOS fails with error: '_SH_DENYNO' was not declared in this scope due to missing #include <share.h> in the file_access_windows.cpp.

akien-mga added a commit to akien-mga/godot that referenced this pull request Aug 10, 2021
@akien-mga
Copy link
Member

MinGW (9.0.0, GCC 11.2.0) build on macOS fails with error: '_SH_DENYNO' was not declared in this scope due to missing #include <share.h> in the file_access_windows.cpp.

Fixed by #51470.

@akien-mga
Copy link
Member

Cherry-picked for 3.4.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Aug 11, 2021
akien-mga added a commit to akien-mga/godot that referenced this pull request Aug 12, 2021
@akien-mga
Copy link
Member

Cherry-picked for 3.3.3.

akien-mga added a commit that referenced this pull request Aug 13, 2021
Follow-up to #51430.

(cherry picked from commit cb52f2c)
sairam4123 pushed a commit to sairam4123/godot that referenced this pull request Nov 10, 2021
lekoder pushed a commit to KoderaSoftwareUnlimited/godot that referenced this pull request Dec 18, 2021
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.

Pressing F5 sometimes opens project manager due to Windows file locking limitations
4 participants