Conversation
_wfopen_s is fixed at _SH_SECURE, so every file the agent wrote stayed locked against all readers for the life of the FILE*, and any handle whose mode lacked an N was inherited by every spawned child. ILibFile_Open() replaces the all open sites. It uses CreateFileW plus _open_osfhandle and _fdopen on Windows, because _wfopen_s() and _wfsopen() cannot grant FILE_SHARE_DELETE. Always binary, never inherited, and the mode letters r w a + x b N are checked on both platforms. The .db keeps its own open, one writer with readers allowed, which is what the datastore needs.
For 'a' modes the handle gets FILE_APPEND_DATA without FILE_WRITE_DATA, so the kernel places every write at the end and two appending processes cannot interleave, where the CRT's seek-then-write leaves a window. Access masks move to FILE_GENERIC_READ/WRITE, since the swap needs the specific rights.
The Windows path of the datastore open already used the same CreateFileW, _open_osfhandle and _fdopen sequence that ILibFile_Open uses, and the POSIX path used plain fopen. Both now go through ILibFile_Open, first with rb+ and, if the file does not exist, with wb+x. Only one process may write, readers are allowed. This is done with a non-blocking exclusive lock. Flock() on POSIX and LockFileEx() on windows on one byte beyond the end of the file, so that readers are never blocked. The .db is also no longer passed on to child processes. Before, a child that was still running after the agent had stopped kept the lock on the .db, so the next agent start could not open it. It would open an empty, cache-only .db and create a new certificate, which appears as a new device on the server.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
All fileopens now go through a shared function which checks the mode and opens files like Node does:
opens every file binary, shared for read, write and delete, non-inheritable by child processes and accept only r(ead), w(rite), a(ppend), +(read and write), x(fail if exists) for mode.
POSIX supports this out of the box (Node's fs is modelled after POSIX after all), except non-inheritable, which is the fcntl(FD_CLOEXEC) after the fopen.
'rx' is refused with EINVAL on every platform, as Node doesn't have that mode.
On windows, _wfopen_s and _wfsopen() lack FILE_SHARE_DELETE, so to circumvent that and get the Node functionality,
the handle is made with CreateFileW (which does accept all share flags), get a filedescriptor with _open_osfhandle() and put into FILE* f afterwards.
This should fix most sharing violation issues on windows.
#edit 10-09: add datastore fix
This is related to the fileopen fix. By using the new ILibFile_Open function for opening the datastore (the local .db), it no longer is passed on to child precesses. If the agent restarted and a chil process was still running, holding a lock on the.db, this would cause the meshagent to open an empty, cache-only .db and create an new certificate. And a new id on the meshserver side.
Details
I understand that I am responsible for and able to explain every line of code I submit.
the impact on platforms and architectures I could not test.
modules/, I re-embedded them so the compiled-in copies inmicroscript/ILibDuktape_Polyfills.cmatch (the agent runs the embedded copies, not the files on disk)..mshoptions table in readme.md.Testing
Tested on linux x86/x64 and windows x86/x64
Powershell test as mentioned in #7832 works, I can open edit, download and copy the file.
#edit 10-09: datastore tests done - push new core, service restart, force agent update on linux/windows.