Skip to content

Commit

Permalink
Fix network file wrong modification detection (regression from v8.7.1)
Browse files Browse the repository at this point in the history 
- increased DEFAULT_MILLISEC constant from 1000 to 3000 (some slower networks obviously cannot respond within the previous 1sec response timeout).
- the problematic situation was apparently made worse by the fact that the Buffer::checkFileState() did not issue one GetFileAttributesEx call but also an unnecessary second one (this was probably caused by an incorrect transcription of the previous version of this func for the new non-blocking thread method of calling the GetFileAttributesEx).
- other problem was that the checking func did not check at all whether the threaded call to the GetFileAttributesEx succeeded or had to be forcibly terminated from the main app thread and expected successful calling only.
- the nppLogNetworkDriveIssue logging has been enhanced.
- added option to check possible WIN32API error code of the threaded GetFileAttributesEx call from the main N++ thread.

Fix notepad-plus-plus#15819, close notepad-plus-plus#15936
  • Loading branch information
@xomx @donho
xomx authored and donho committed Dec 19, 2024
1 parent 0c4bb24 commit de1a040
Show file tree
Hide file tree
Showing 4 changed files with 138 additions and 43 deletions.
14 changes: 11 additions & 3 deletions PowerEditor/src/MISC/Common/Common.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1751,7 +1751,7 @@ bool Version::isCompatibleTo(const Version& from, const Version& to) const
}


#define DEFAULT_MILLISEC 1000
#define DEFAULT_MILLISEC 3000


//----------------------------------------------------
Expand Down Expand Up @@ -1816,6 +1816,7 @@ struct GetAttrExParamResult
wstring _filePath;
WIN32_FILE_ATTRIBUTE_DATA _attributes{};
BOOL _result = FALSE;
DWORD _error = NO_ERROR;
bool _isTimeoutReached = true;

GetAttrExParamResult(wstring filePath): _filePath(filePath) {
Expand All @@ -1826,12 +1827,16 @@ struct GetAttrExParamResult
DWORD WINAPI getFileAttributesExWorker(void* data)
{
GetAttrExParamResult* inAndOut = static_cast<GetAttrExParamResult*>(data);
::SetLastError(NO_ERROR);
inAndOut->_result = ::GetFileAttributesExW(inAndOut->_filePath.c_str(), GetFileExInfoStandard, &(inAndOut->_attributes));
if (!(inAndOut->_result))
inAndOut->_error = ::GetLastError();
inAndOut->_isTimeoutReached = false;
return ERROR_SUCCESS;
};

BOOL getFileAttributesExWithTimeout(const wchar_t* filePath, WIN32_FILE_ATTRIBUTE_DATA* fileAttr, DWORD milliSec2wait, bool* isTimeoutReached)
BOOL getFileAttributesExWithTimeout(const wchar_t* filePath, WIN32_FILE_ATTRIBUTE_DATA* fileAttr,
DWORD milliSec2wait, bool* isTimeoutReached, DWORD* pdwWin32ApiError)
{
GetAttrExParamResult data(filePath);

Expand All @@ -1855,13 +1860,16 @@ BOOL getFileAttributesExWithTimeout(const wchar_t* filePath, WIN32_FILE_ATTRIBUT
::TerminateThread(hThread, dwWaitStatus);
break;
}
CloseHandle(hThread);
::CloseHandle(hThread);

*fileAttr = data._attributes;

if (isTimeoutReached != nullptr)
*isTimeoutReached = data._isTimeoutReached;

if (pdwWin32ApiError != nullptr)
*pdwWin32ApiError = data._error;

return data._result;
}

Expand Down
6 changes: 4 additions & 2 deletions PowerEditor/src/MISC/Common/Common.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -280,8 +280,10 @@ class Version final
};


BOOL getDiskFreeSpaceWithTimeout(const wchar_t* dirPath, ULARGE_INTEGER* freeBytesForUser, DWORD milliSec2wait = 0, bool* isTimeoutReached = nullptr);
BOOL getFileAttributesExWithTimeout(const wchar_t* filePath, WIN32_FILE_ATTRIBUTE_DATA* fileAttr, DWORD milliSec2wait = 0, bool* isTimeoutReached = nullptr);
BOOL getDiskFreeSpaceWithTimeout(const wchar_t* dirPath, ULARGE_INTEGER* freeBytesForUser,
DWORD milliSec2wait = 0, bool* isTimeoutReached = nullptr);
BOOL getFileAttributesExWithTimeout(const wchar_t* filePath, WIN32_FILE_ATTRIBUTE_DATA* fileAttr,
DWORD milliSec2wait = 0, bool* isTimeoutReached = nullptr, DWORD* pdwWin32ApiError = nullptr);

bool doesFileExist(const wchar_t* filePath, DWORD milliSec2wait = 0, bool* isTimeoutReached = nullptr);
bool doesDirectoryExist(const wchar_t* dirPath, DWORD milliSec2wait = 0, bool* isTimeoutReached = nullptr);
Expand Down
1 change: 0 additions & 1 deletion PowerEditor/src/NppBigSwitch.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2191,7 +2191,6 @@ LRESULT Notepad_plus::process(HWND hwnd, UINT message, WPARAM wParam, LPARAM lPa
if (nppgui._fileAutoDetection != cdDisabled)
{
bool bCheckOnlyCurrentBuffer = (nppgui._fileAutoDetection & cdEnabledNew) ? true : false;

checkModifiedDocument(bCheckOnlyCurrentBuffer);
return TRUE;
}
Expand Down
160 changes: 123 additions & 37 deletions PowerEditor/src/ScintillaComponent/Buffer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,12 +142,49 @@ void Buffer::setLangType(LangType lang, const wchar_t* userLangName)
void Buffer::updateTimeStamp()
{
FILETIME timeStampLive {};

WIN32_FILE_ATTRIBUTE_DATA attributes{};
attributes.dwFileAttributes = INVALID_FILE_ATTRIBUTES;
if (getFileAttributesExWithTimeout(_fullPathName.c_str(), &attributes))
bool bWorkerThreadTerminated = true;
DWORD dwWin32ApiError = NO_ERROR;
BOOL bGetFileAttributesExSucceeded = getFileAttributesExWithTimeout(_fullPathName.c_str(), &attributes, 0, &bWorkerThreadTerminated, &dwWin32ApiError);
if (bGetFileAttributesExSucceeded && (attributes.dwFileAttributes != INVALID_FILE_ATTRIBUTES) && !(attributes.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY))
{
timeStampLive = attributes.ftLastWriteTime;
}
else
{
if (_currentStatus != DOC_UNNAMED)
{
NppParameters& nppParam = NppParameters::getInstance();
if (nppParam.doNppLogNetworkDriveIssue())
{
wstring issueFn = nppLogNetworkDriveIssue;
issueFn += L".log";
wstring nppIssueLog = nppParam.getUserPath();
pathAppend(nppIssueLog, issueFn);
std::string msg = wstring2string(_fullPathName, CP_UTF8);
msg += " in Buffer::updateTimeStamp(), getFileAttributesExWithTimeout returned ";
if (bGetFileAttributesExSucceeded)
msg += "TRUE";
else
msg += "FALSE";
if (bWorkerThreadTerminated)
{
msg += ", its worker thread had to be forcefully terminated due to timeout reached!";
}
else
{
msg += ", its worker thread finished successfully within the timeout given, ";
if (attributes.dwFileAttributes == INVALID_FILE_ATTRIBUTES)
msg += "dwFileAttributes == INVALID_FILE_ATTRIBUTES ! (WIN32API Error Code: " + std::to_string(dwWin32ApiError) + ")";
else
msg += "dwFileAttributes has the FILE_ATTRIBUTE_DIRECTORY flag set!";
}
writeLog(nppIssueLog.c_str(), msg.c_str());
}
}
}

LONG res = CompareFileTime(&_timeStamp, &timeStampLive);
if (res == -1 || res == 1)
Expand All @@ -157,19 +194,28 @@ void Buffer::updateTimeStamp()
// It can happen when user copies a backup of editing file somewhere-else firstly, then modifies the editing file in Notepad++ and saves it.
// Now user copies the backup back to erase the modified editing file outside Notepad++ (via Explorer).
{
if (res == 1)
NppParameters& nppParam = NppParameters::getInstance();
if (nppParam.doNppLogNetworkDriveIssue())
{
NppParameters& nppParam = NppParameters::getInstance();
if (nppParam.doNppLogNetworkDriveIssue())
char buf[1024]{};
if (res == 1)
{
sprintf_s(buf, _countof(buf) - 1, " in updateTimeStamp(): timeStampLive (%lu/%lu) < _timeStamp (%lu/%lu)",
timeStampLive.dwLowDateTime, timeStampLive.dwHighDateTime, _timeStamp.dwLowDateTime, _timeStamp.dwHighDateTime);
}
else
{
// usual case, uncomment if needed to log too
//sprintf_s(buf, _countof(buf) - 1, " in updateTimeStamp(): timeStampLive (%lu/%lu) > _timeStamp (%lu/%lu)",
// timeStampLive.dwLowDateTime, timeStampLive.dwHighDateTime, _timeStamp.dwLowDateTime, _timeStamp.dwHighDateTime);
}
if (buf[0] != '\0')
{
wstring issueFn = nppLogNetworkDriveIssue;
issueFn += L".log";
wstring nppIssueLog = nppParam.getUserPath();
pathAppend(nppIssueLog, issueFn);

std::string msg = wstring2string(_fullPathName, CP_UTF8);
char buf[1024];
sprintf(buf, " in updateTimeStamp(): timeStampLive (%lu/%lu) < _timeStamp (%lu/%lu)", timeStampLive.dwLowDateTime, timeStampLive.dwHighDateTime, _timeStamp.dwLowDateTime, _timeStamp.dwHighDateTime);
msg += buf;
writeLog(nppIssueLog.c_str(), msg.c_str());
}
Expand Down Expand Up @@ -260,10 +306,14 @@ bool Buffer::checkFileState() // returns true if the status has been changed (it
if (_currentStatus == DOC_UNNAMED || isMonitoringOn())
return false;

NppParameters& nppParam = NppParameters::getInstance();

WIN32_FILE_ATTRIBUTE_DATA attributes{};
attributes.dwFileAttributes = INVALID_FILE_ATTRIBUTES;
NppParameters& nppParam = NppParameters::getInstance();
bool fileExists = doesFileExist(_fullPathName.c_str());
bool bWorkerThreadTerminated = true;
DWORD dwWin32ApiError = NO_ERROR;
BOOL bGetFileAttributesExSucceeded = getFileAttributesExWithTimeout(_fullPathName.c_str(), &attributes, 0, &bWorkerThreadTerminated, &dwWin32ApiError);
bool fileExists = (bGetFileAttributesExSucceeded && (attributes.dwFileAttributes != INVALID_FILE_ATTRIBUTES) && !(attributes.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY));

#ifndef _WIN64
bool isWow64Off = false;
Expand All @@ -272,10 +322,42 @@ bool Buffer::checkFileState() // returns true if the status has been changed (it
nppParam.safeWow64EnableWow64FsRedirection(FALSE);
isWow64Off = true;

fileExists = doesFileExist(_fullPathName.c_str());
bGetFileAttributesExSucceeded = getFileAttributesExWithTimeout(_fullPathName.c_str(), &attributes, 0, &bWorkerThreadTerminated, &dwWin32ApiError);
fileExists = (bGetFileAttributesExSucceeded && (attributes.dwFileAttributes != INVALID_FILE_ATTRIBUTES) && !(attributes.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY));
}
#endif

if (!fileExists && nppParam.doNppLogNetworkDriveIssue())
{
wstring issueFn = nppLogNetworkDriveIssue;
issueFn += L".log";
wstring nppIssueLog = nppParam.getUserPath();
pathAppend(nppIssueLog, issueFn);
std::string msg = wstring2string(_fullPathName, CP_UTF8);
msg += " in Buffer::checkFileState(), getFileAttributesExWithTimeout returned ";
if (bGetFileAttributesExSucceeded)
{
msg += "TRUE";
}
else
{
msg += "FALSE";
}
if (bWorkerThreadTerminated)
{
msg += ", its worker thread had to be forcefully terminated due to timeout reached!";
}
else
{
msg += ", its worker thread finished successfully within the timeout given, ";
if (attributes.dwFileAttributes == INVALID_FILE_ATTRIBUTES)
msg += "dwFileAttributes == INVALID_FILE_ATTRIBUTES ! (WIN32API Error Code: " + std::to_string(dwWin32ApiError) + ")";
else
msg += "dwFileAttributes has the FILE_ATTRIBUTE_DIRECTORY flag set!";
}
writeLog(nppIssueLog.c_str(), msg.c_str());
}

bool isOK = false;
if (_currentStatus == DOC_INACCESSIBLE && !fileExists) //document is absent on its first load - we set readonly and not dirty, and make it be as document which has been deleted
{
Expand All @@ -291,29 +373,25 @@ bool Buffer::checkFileState() // returns true if the status has been changed (it
{
_currentStatus = DOC_DELETED;
_isFileReadOnly = false;
_isDirty = true; //dirty sicne no match with filesystem
_isDirty = true; //dirty since no match with filesystem
_timeStamp = {};
doNotify(BufferChangeStatus | BufferChangeReadonly | BufferChangeTimestamp);
isOK = true;
}
else if (_currentStatus == DOC_DELETED && fileExists) //document has returned from its grave
{
if (GetFileAttributesEx(_fullPathName.c_str(), GetFileExInfoStandard, &attributes) != 0) // fileExists so it's safe to call GetFileAttributesEx directly
// fileExists==true here means that we can use the previously obtained attributes safely
_isFileReadOnly = attributes.dwFileAttributes & FILE_ATTRIBUTE_READONLY;
_currentStatus = DOC_MODIFIED;
_timeStamp = attributes.ftLastWriteTime;
if (_reloadFromDiskRequestGuard.try_lock())
{
_isFileReadOnly = attributes.dwFileAttributes & FILE_ATTRIBUTE_READONLY;

_currentStatus = DOC_MODIFIED;
_timeStamp = attributes.ftLastWriteTime;

if (_reloadFromDiskRequestGuard.try_lock())
{
doNotify(BufferChangeStatus | BufferChangeReadonly | BufferChangeTimestamp);
_reloadFromDiskRequestGuard.unlock();
}
isOK = true;
doNotify(BufferChangeStatus | BufferChangeReadonly | BufferChangeTimestamp);
_reloadFromDiskRequestGuard.unlock();
}
isOK = true;
}
else if (getFileAttributesExWithTimeout(_fullPathName.c_str(), &attributes))
else if (bGetFileAttributesExSucceeded)
{
int mask = 0; //status always 'changes', even if from modified to modified
bool isFileReadOnly = attributes.dwFileAttributes & FILE_ATTRIBUTE_READONLY;
Expand All @@ -324,30 +402,39 @@ bool Buffer::checkFileState() // returns true if the status has been changed (it
}

LONG res = CompareFileTime(&_timeStamp, &attributes.ftLastWriteTime);

if (res == -1 || res == 1)
// (res == -1) => attributes.ftLastWriteTime is later, it means the file has been modified outside of Notepad++ - usual case
//
// (res == 1) => The timestamp get directly from the file on disk is earlier than buffer's timestamp - unusual case
// It can happen when user copies a backup of editing file somewhere-else firstly, then modifies the editing file in Notepad++ and saves it.
// Now user copies the backup back to erase the modified editing file outside Notepad++ (via Explorer).
// (res == -1) => attributes.ftLastWriteTime is later, it means the file has been modified outside of Notepad++ - usual case
//
// (res == 1) => The timestamp get directly from the file on disk is earlier than buffer's timestamp - unusual case
// It can happen when user copies a backup of editing file somewhere-else firstly, then modifies the editing file in Notepad++ and saves it.
// Now user copies the backup back to erase the modified editing file outside Notepad++ (via Explorer).
{
if (res == 1)
if (nppParam.doNppLogNetworkDriveIssue())
{
if (nppParam.doNppLogNetworkDriveIssue())
char buf[1024]{};
if (res == 1)
{
sprintf_s(buf, _countof(buf) - 1, " in checkFileState(): attributes.ftLastWriteTime (%lu/%lu) < _timeStamp (%lu/%lu)",
attributes.ftLastWriteTime.dwLowDateTime, attributes.ftLastWriteTime.dwHighDateTime, _timeStamp.dwLowDateTime, _timeStamp.dwHighDateTime);
}
else
{
// usual state after a file modification, uncomment if needed to log too
//sprintf_s(buf, _countof(buf) - 1, " in checkFileState(): attributes.ftLastWriteTime (%lu/%lu) > _timeStamp (%lu/%lu)",
// attributes.ftLastWriteTime.dwLowDateTime, attributes.ftLastWriteTime.dwHighDateTime, _timeStamp.dwLowDateTime, _timeStamp.dwHighDateTime);
}
if (buf[0] != '\0')
{
wstring issueFn = nppLogNetworkDriveIssue;
issueFn += L".log";
wstring nppIssueLog = nppParam.getUserPath();
pathAppend(nppIssueLog, issueFn);

std::string msg = wstring2string(_fullPathName, CP_UTF8);
char buf[1024];
sprintf(buf, " in checkFileState(): attributes.ftLastWriteTime (%lu/%lu) < _timeStamp (%lu/%lu)", attributes.ftLastWriteTime.dwLowDateTime, attributes.ftLastWriteTime.dwHighDateTime, _timeStamp.dwLowDateTime, _timeStamp.dwHighDateTime);
msg += buf;
writeLog(nppIssueLog.c_str(), msg.c_str());
}
}

_timeStamp = attributes.ftLastWriteTime;
mask |= BufferChangeTimestamp;
_currentStatus = DOC_MODIFIED;
Expand All @@ -360,9 +447,7 @@ bool Buffer::checkFileState() // returns true if the status has been changed (it
if (_reloadFromDiskRequestGuard.try_lock())
{
doNotify(mask);

_reloadFromDiskRequestGuard.unlock();

return true;
}
}
Expand All @@ -376,6 +461,7 @@ bool Buffer::checkFileState() // returns true if the status has been changed (it
nppParam.safeWow64EnableWow64FsRedirection(TRUE);
}
#endif

return isOK;
}

Expand Down

0 comments on commit de1a040

Please sign in to comment.