-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
possible crash when recursively entering the Notepad++ backup_mutex #14906
Comments
This commit revert (kind of) 581aff6 Fix notepad-plus-plus#14906
@xomx |
Currently I cannot. |
@xomx |
Description of the Issue
While simulating the stalled/blocked/hanged file saving / filecache flushing for the NUL-corruptions issues debugging purposes, I found a bad N++ design - under some circumstances like that blocked file-IO, there is a possibility of multiple entry to the N++
backup_mutex
std::mutex
from the same thread, resulting in a N++ crash.Ref: https://en.cppreference.com/w/cpp/thread/mutex/lock
"If lock is called by a thread that already owns the std::mutex, the behavior is undefined: for example, the program may deadlock.
An implementation that can detect the invalid usage is encouraged to throw a std::system_error with error condition
resource_deadlock_would_occur instead of deadlocking."
(Note: In such a case MS C++ throws the _RESOURCE_DEADLOCK_WOULD_OCCUR)
This can lead to loss or corruption of the opened/saved files in N++, as well as failure to save some important configuration files, which are saved when the N++ is being closed (e.g. the session.xml, config.xml, etc.).
STR
Current N++ design allows calling of its
FileManager::backupCurrentBuffer()
func multiple times from the same thread even before finishing the backup saving there but the file-IO could be slow/blocked, e.g.:and the file to be saved is bigger and the disk used is not fast enough
Win32_IO_File::write
orWin32_IO_File::close()
Expected Behavior
N++ handles that (temporarily) "blocked-IO" situation without the crash.
Actual Behavior
N++ will start to pointlessly stack up its
NPPM_INTERNAL_SAVEBACKUP
messages via the periodically calledPostMessage
in theNotepad_plus::backupDocument(void * /*param*/)
no matter if the backup file-saving can be / is completed or not, theFileManager::backupCurrentBuffer()
will be reentered from the same thread causing crash at thestd::lock_guard<std::mutex> lock(backup_mutex)
.Proposed solutions
std::mutex
bystd::recursive_mutex
. This will only delay the possible crash of the app. Thestd::recursive_mutex
has not its own locking counter method (and its native_handle() method is unfortunately not implemented on the Windows platform) needed for a proper solution.lockcount()
method, as the correct solution is the immediate returning from thatFileManager::backupCurrentBuffer()
func when a recursive lock-situation (not finished previous backup) is detected.@donho
If you agree with the above no.2 proposal, assign me please to this issue, I will try a PR.
The text was updated successfully, but these errors were encountered: