From de70fa5b2fa783fa5c4d0f9f9cfbeb6d11101d0a Mon Sep 17 00:00:00 2001 From: Lauri Nurmi Date: Mon, 24 Oct 2016 21:56:11 +0300 Subject: [PATCH 1/5] Fix a possible out-of-bounds access to an array --- src/common/log.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/common/log.cpp b/src/common/log.cpp index 94a6aede7b..35ffb86ba6 100644 --- a/src/common/log.cpp +++ b/src/common/log.cpp @@ -1095,7 +1095,7 @@ const wxChar *wxSysErrorMsg(unsigned long nErrCode) // returned string is capitalized and ended with '\r\n' - bad s_szBuf[0] = (wxChar)wxTolower(s_szBuf[0]); size_t len = wxStrlen(s_szBuf); - if ( len > 0 ) { + if ( len >= 2 ) { // truncate string if ( s_szBuf[len - 2] == wxS('\r') ) s_szBuf[len - 2] = wxS('\0'); From 3926538fea9b584e3175ee46049ca1e433c70a32 Mon Sep 17 00:00:00 2001 From: Lauri Nurmi Date: Mon, 24 Oct 2016 22:01:34 +0300 Subject: [PATCH 2/5] Don't de-capitalize system error messages on MSW The returned string being capitalized is not 'bad' as the ancient comment from the 90s suggested. At least on Windows 7+, system error messages are full sentences beginning with a capital letter, ending in a full stop; there is no point in lowercasing the first letter. --- src/common/log.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/common/log.cpp b/src/common/log.cpp index 35ffb86ba6..c2828ba8f5 100644 --- a/src/common/log.cpp +++ b/src/common/log.cpp @@ -1092,8 +1092,7 @@ const wxChar *wxSysErrorMsg(unsigned long nErrCode) LocalFree(lpMsgBuf); - // returned string is capitalized and ended with '\r\n' - bad - s_szBuf[0] = (wxChar)wxTolower(s_szBuf[0]); + // returned string is ended with '\r\n' - bad size_t len = wxStrlen(s_szBuf); if ( len >= 2 ) { // truncate string From 343318d73ed24b1d94ddeaafa9339c38c947dcbd Mon Sep 17 00:00:00 2001 From: Lauri Nurmi Date: Tue, 25 Oct 2016 12:01:03 +0300 Subject: [PATCH 3/5] Add a thread-safe wxSysErrorMsgStr() Implement wxSysErrorMsg's functionality without using static buffers; have the caller provide the buffer. When the caller uses the original API and does not provide a buffer, a static buffer is still used. wxSysErrorMsgStr() returns a wxString. Also use strerror_r() instead of strerror() on platforms other than MSW. --- include/wx/log.h | 4 ++++ interface/wx/log.h | 17 +++++++++++++ src/common/log.cpp | 60 +++++++++++++++++++++++++++++++++------------- 3 files changed, 64 insertions(+), 17 deletions(-) diff --git a/include/wx/log.h b/include/wx/log.h index cbc15efde9..3228bed7ff 100644 --- a/include/wx/log.h +++ b/include/wx/log.h @@ -1188,6 +1188,9 @@ WXDLLIMPEXP_BASE unsigned long wxSysErrorCode(); // return the error message for given (or last if 0) error code WXDLLIMPEXP_BASE const wxChar* wxSysErrorMsg(unsigned long nErrCode = 0); +// return the error message for given (or last if 0) error code +WXDLLIMPEXP_BASE wxString wxSysErrorMsgStr(unsigned long nErrCode = 0); + // ---------------------------------------------------------------------------- // define wxLog() functions which can be used by application instead of // stdio, iostream &c for log messages for easy redirection @@ -1393,6 +1396,7 @@ public: // Dummy macros to replace some functions. #define wxSysErrorCode() (unsigned long)0 #define wxSysErrorMsg( X ) (const wxChar*)NULL +#define wxSysErrorMsgStr( X ) wxEmptyString // Fake symbolic trace masks... for those that are used frequently #define wxTRACE_OleCalls wxEmptyString // OLE interface calls diff --git a/interface/wx/log.h b/interface/wx/log.h index 7b5dc8de73..b76d734730 100644 --- a/interface/wx/log.h +++ b/interface/wx/log.h @@ -1226,6 +1226,23 @@ unsigned long wxSysErrorCode(); @a errCode is 0 (default), the last error code (as returned by wxSysErrorCode()) is used. + Use this function instead of wxSysErrorMsg(), as the latter one is not + thread-safe. + + @see wxSysErrorCode(), wxLogSysError() + + @header{wx/log.h} +*/ +wxString wxSysErrorMsgStr(unsigned long errCode = 0); + +/** + Returns the error message corresponding to the given system error code. If + @a errCode is 0 (default), the last error code (as returned by + wxSysErrorCode()) is used. + + Use wxSysErrorMsgStr() instead of this function especially in a + multi-threaded application. + @see wxSysErrorCode(), wxLogSysError() @header{wx/log.h} diff --git a/src/common/log.cpp b/src/common/log.cpp index c2828ba8f5..7c1e9e89bb 100644 --- a/src/common/log.cpp +++ b/src/common/log.cpp @@ -48,6 +48,8 @@ // other standard headers #include +#include + #include #include @@ -1055,15 +1057,12 @@ unsigned long wxSysErrorCode() #endif //Win/Unix } -// get error message from system -const wxChar *wxSysErrorMsg(unsigned long nErrCode) +static const wxChar* GetSysErrorMsg(wxChar* szBuf, size_t len, unsigned long nErrCode) { if ( nErrCode == 0 ) nErrCode = wxSysErrorCode(); #if defined(__WINDOWS__) - static wxChar s_szBuf[1024]; - // get error message from system LPVOID lpMsgBuf; if ( ::FormatMessage @@ -1079,8 +1078,8 @@ const wxChar *wxSysErrorMsg(unsigned long nErrCode) { // if this happens, something is seriously wrong, so don't use _() here // for safety - wxSprintf(s_szBuf, wxS("unknown error %lx"), nErrCode); - return s_szBuf; + wxSprintf(szBuf, wxS("unknown error %lx"), nErrCode); + return szBuf; } @@ -1088,34 +1087,61 @@ const wxChar *wxSysErrorMsg(unsigned long nErrCode) // Crashes on SmartPhone (FIXME) if( lpMsgBuf != 0 ) { - wxStrlcpy(s_szBuf, (const wxChar *)lpMsgBuf, WXSIZEOF(s_szBuf)); + wxStrlcpy(szBuf, (const wxChar *)lpMsgBuf, len); LocalFree(lpMsgBuf); // returned string is ended with '\r\n' - bad - size_t len = wxStrlen(s_szBuf); + size_t len = wxStrlen(szBuf); if ( len >= 2 ) { // truncate string - if ( s_szBuf[len - 2] == wxS('\r') ) - s_szBuf[len - 2] = wxS('\0'); + if ( szBuf[len - 2] == wxS('\r') ) + szBuf[len - 2] = wxS('\0'); } } else { - s_szBuf[0] = wxS('\0'); + szBuf[0] = wxS('\0'); } - return s_szBuf; + return szBuf; #else // !__WINDOWS__ + char buffer[1024]; + char *errorMsg = buffer; + +#ifdef _GNU_SOURCE // GNU-specific strerror_r + // GNU's strerror_r has a weird interface -- it doesn't + // necessarily copy anything to the buffer given; use return + // value instead. + errorMsg = strerror_r((int)nErrCode, buffer, sizeof(buffer)); +#else // XSI-compliant strerror_r + strerror_r((int)nErrCode, buffer, sizeof(buffer)); +#endif + + // at this point errorMsg might not point to buffer anymore + szBuf[0] = wxS('\0'); #if wxUSE_UNICODE - static wchar_t s_wzBuf[1024]; - wxConvCurrent->MB2WC(s_wzBuf, strerror((int)nErrCode), - WXSIZEOF(s_wzBuf) - 1); - return s_wzBuf; + wxConvCurrent->MB2WC(szBuf, errorMsg, len - 1); + szBuf[len - 1] = wxS('\0'); #else - return strerror((int)nErrCode); + wxStrlcpy(szBuf, errorMsg, len); #endif + return szBuf; #endif // __WINDOWS__/!__WINDOWS__ } +// get error message from system +const wxChar *wxSysErrorMsg(unsigned long nErrCode) +{ + static wxChar s_szBuf[1024]; + return GetSysErrorMsg(s_szBuf, WXSIZEOF(s_szBuf), nErrCode); +} + +// get error message from system as wxString +wxString wxSysErrorMsgStr(unsigned long nErrCode) +{ + wxChar szBuf[1024]; + return GetSysErrorMsg(szBuf, WXSIZEOF(szBuf), nErrCode); +} + #endif // wxUSE_LOG From 902130f64e7a3daab3db7691338af4e1f6c0a00d Mon Sep 17 00:00:00 2001 From: Lauri Nurmi Date: Mon, 21 Nov 2016 18:37:25 +0200 Subject: [PATCH 4/5] Use the new wxSysErrorMsgStr() instead of wxSysErrorMsg() --- include/wx/log.h | 4 ++-- src/common/log.cpp | 2 +- src/common/strconv.cpp | 4 ++-- src/msw/debughlp.cpp | 2 +- src/msw/dialup.cpp | 4 ++-- src/msw/secretstore.cpp | 8 ++++---- src/msw/thread.cpp | 2 +- 7 files changed, 13 insertions(+), 13 deletions(-) diff --git a/include/wx/log.h b/include/wx/log.h index 3228bed7ff..7f0112c04f 100644 --- a/include/wx/log.h +++ b/include/wx/log.h @@ -1479,13 +1479,13 @@ wxSafeShowMessage(const wxString& title, const wxString& text); #define wxLogApiError(api, rc) \ wxLogDebug(wxT("%s(%d): '%s' failed with error 0x%08lx (%s)."), \ __FILE__, __LINE__, api, \ - (long)rc, wxSysErrorMsg(rc)) + (long)rc, wxSysErrorMsgStr(rc)) #else // !VC++ #define wxLogApiError(api, rc) \ wxLogDebug(wxT("In file %s at line %d: '%s' failed with ") \ wxT("error 0x%08lx (%s)."), \ __FILE__, __LINE__, api, \ - (long)rc, wxSysErrorMsg(rc)) + (long)rc, wxSysErrorMsgStr(rc)) #endif // VC++/!VC++ #define wxLogLastError(api) wxLogApiError(api, wxSysErrorCode()) diff --git a/src/common/log.cpp b/src/common/log.cpp index 7c1e9e89bb..137c05a0cc 100644 --- a/src/common/log.cpp +++ b/src/common/log.cpp @@ -418,7 +418,7 @@ wxLog::CallDoLogNow(wxLogLevel level, { const long err = static_cast(num); - suffix.Printf(_(" (error %ld: %s)"), err, wxSysErrorMsg(err)); + suffix.Printf(_(" (error %ld: %s)"), err, wxSysErrorMsgStr(err)); } #if wxUSE_LOG_TRACE diff --git a/src/common/strconv.cpp b/src/common/strconv.cpp index 3f0cd2caeb..512d51088e 100644 --- a/src/common/strconv.cpp +++ b/src/common/strconv.cpp @@ -2411,7 +2411,7 @@ wxMBConv_iconv::ToWChar(wchar_t *dst, size_t dstLen, if (ICONV_FAILED(cres, srcLen)) { //VS: it is ok if iconv fails, hence trace only - wxLogTrace(TRACE_STRCONV, wxT("iconv failed: %s"), wxSysErrorMsg(wxSysErrorCode())); + wxLogTrace(TRACE_STRCONV, wxT("iconv failed: %s"), wxSysErrorMsgStr(wxSysErrorCode())); return wxCONV_FAILED; } @@ -2479,7 +2479,7 @@ size_t wxMBConv_iconv::FromWChar(char *dst, size_t dstLen, if (ICONV_FAILED(cres, inbuflen)) { - wxLogTrace(TRACE_STRCONV, wxT("iconv failed: %s"), wxSysErrorMsg(wxSysErrorCode())); + wxLogTrace(TRACE_STRCONV, wxT("iconv failed: %s"), wxSysErrorMsgStr(wxSysErrorCode())); return wxCONV_FAILED; } diff --git a/src/msw/debughlp.cpp b/src/msw/debughlp.cpp index 592595d4a5..a1719f2902 100644 --- a/src/msw/debughlp.cpp +++ b/src/msw/debughlp.cpp @@ -212,7 +212,7 @@ const wxString& wxDbgHelpDLL::GetErrorMessage() void wxDbgHelpDLL::LogError(const wxChar *func) { ::OutputDebugString(wxString::Format(wxT("dbghelp: %s() failed: %s\r\n"), - func, wxSysErrorMsg(::GetLastError())).t_str()); + func, wxSysErrorMsgStr(::GetLastError())).t_str()); } // ---------------------------------------------------------------------------- diff --git a/src/msw/dialup.cpp b/src/msw/dialup.cpp index 6337b847ed..9ac7a9eed1 100644 --- a/src/msw/dialup.cpp +++ b/src/msw/dialup.cpp @@ -470,7 +470,7 @@ wxString wxDialUpManagerMSW::GetErrorString(DWORD error) { case ERROR_INVALID_PARAMETER: // this was a standard Win32 error probably - return wxString(wxSysErrorMsg(error)); + return wxSysErrorMsgStr(error); default: { @@ -1258,7 +1258,7 @@ static DWORD wxRasMonitorThread(wxRasThreadData *data) ( wxT("WaitForMultipleObjects(RasMonitor) failed: 0x%08lx (%s)"), err, - wxSysErrorMsg(err) + wxSysErrorMsgStr(err) ); // no sense in continuing, who knows if the handles we're diff --git a/src/msw/secretstore.cpp b/src/msw/secretstore.cpp index 590816be98..3cc4d696b5 100644 --- a/src/msw/secretstore.cpp +++ b/src/msw/secretstore.cpp @@ -27,7 +27,7 @@ #include "wx/secretstore.h" #include "wx/private/secretstore.h" -#include "wx/log.h" // wxSysErrorMsg() +#include "wx/log.h" // wxSysErrorMsgStr() // Somewhat surprisingly, wincred.h is not self-contained and relies on some // standard Windows macros being defined without including the headers defining @@ -84,7 +84,7 @@ public: if ( !::CredWrite(&cred, 0) ) { - errmsg = wxSysErrorMsg(); + errmsg = wxSysErrorMsgStr(); return false; } @@ -103,7 +103,7 @@ public: // Not having the password for this service/user combination is not // an error, but anything else is. if ( ::GetLastError() != ERROR_NOT_FOUND ) - errmsg = wxSysErrorMsg(); + errmsg = wxSysErrorMsgStr(); return NULL; } @@ -123,7 +123,7 @@ public: { // Same logic as in Load() above. if ( ::GetLastError() != ERROR_NOT_FOUND ) - errmsg = wxSysErrorMsg(); + errmsg = wxSysErrorMsgStr(); return false; } diff --git a/src/msw/thread.cpp b/src/msw/thread.cpp index f696ab9c16..0805e1e19a 100644 --- a/src/msw/thread.cpp +++ b/src/msw/thread.cpp @@ -1371,7 +1371,7 @@ void WXDLLIMPEXP_BASE wxWakeUpMainThread() wxS("Failed to wake up main thread: PostThreadMessage(WM_NULL) ") wxS("failed with error 0x%08lx (%s)."), ec, - wxSysErrorMsg(ec) + wxSysErrorMsgStr(ec) ); } } From 9e24755d00599ec0f35542beea694ea2816753dc Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sun, 27 Nov 2016 15:02:53 +0100 Subject: [PATCH 5/5] Minor improvements to wxSysErrorMsgStr() documentation Link to the new function, instead of wxSysErrorMsg(), from the other functions documentation. Also mention that this function is new since 3.1.0. --- interface/wx/log.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/interface/wx/log.h b/interface/wx/log.h index b76d734730..8b8d59e328 100644 --- a/interface/wx/log.h +++ b/interface/wx/log.h @@ -1215,7 +1215,7 @@ void wxSafeShowMessage(const wxString& title, const wxString& text); Returns the error code from the last system call. This function uses @c errno on Unix platforms and @c GetLastError under Win32. - @see wxSysErrorMsg(), wxLogSysError() + @see wxSysErrorMsgStr(), wxLogSysError() @header{wx/log.h} */ @@ -1229,6 +1229,8 @@ unsigned long wxSysErrorCode(); Use this function instead of wxSysErrorMsg(), as the latter one is not thread-safe. + @since 3.1.0 + @see wxSysErrorCode(), wxLogSysError() @header{wx/log.h} @@ -1457,7 +1459,7 @@ void wxVLogStatus(const char* formatString, va_list argPtr); form of this function takes the error code explicitly as the first argument. - @see wxSysErrorCode(), wxSysErrorMsg() + @see wxSysErrorCode(), wxSysErrorMsgStr() @header{wx/log.h} */