From c47acbeb523420fc954308609bb1d6ccf476bd9f Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Thu, 2 Nov 2017 01:19:01 +0100 Subject: [PATCH] Fix wxMBConv::cWC2MB() and cMB2WC() returned buffer length This commit refactors the overloads of cMB2WC() and cWC2MB() methods taking raw pointers and buffers to reuse the same code and fixes the wrong length of the buffer returned by cWC2MB(wchar_t*) overload for conversions using multiple bytes to represent the NUL terminator character (it previously was wrong for UTF-16 and UTF-32 conversions due to wrongly subtracting 1 from the length when creating it instead of correctly subtracting GetMBNulLen()) and the wrong length of the buffer returned from cMB2WC(char*) overload where no adjustment for the trailing NUL was done at all. Also return simple default-constructed buffers from these methods in case of failure instead of using wxScopedCharBuffer::CreateNonOwned() which is less obvious and less efficient (even if the latter probably doesn't matter here because it's only done in case of an error). Finally, add tests checking that using WC2MB() or either of cWC2MB() overloads returns the buffers of the same length and with the same contents. --- include/wx/strconv.h | 17 ++++++--- src/common/strconv.cpp | 69 ++++++++++++++----------------------- tests/mbconv/mbconvtest.cpp | 43 +++++++++++++++++++++++ 3 files changed, 81 insertions(+), 48 deletions(-) diff --git a/include/wx/strconv.h b/include/wx/strconv.h index 6822ca62d1..207a235605 100644 --- a/include/wx/strconv.h +++ b/include/wx/strconv.h @@ -74,11 +74,15 @@ public: // Convenience functions for translating NUL-terminated strings: return // the buffer containing the converted string or empty buffer if the // conversion failed. - wxWCharBuffer cMB2WC(const char *in) const; - wxCharBuffer cWC2MB(const wchar_t *in) const; + wxWCharBuffer cMB2WC(const char *in) const + { return DoConvertMB2WC(in, wxNO_LEN); } + wxCharBuffer cWC2MB(const wchar_t *in) const + { return DoConvertWC2MB(in, wxNO_LEN); } - wxWCharBuffer cMB2WC(const wxScopedCharBuffer& in) const; - wxCharBuffer cWC2MB(const wxScopedWCharBuffer& in) const; + wxWCharBuffer cMB2WC(const wxScopedCharBuffer& in) const + { return DoConvertMB2WC(in, in.length()); } + wxCharBuffer cWC2MB(const wxScopedWCharBuffer& in) const + { return DoConvertWC2MB(in, in.length()); } // Convenience functions for converting strings which may contain embedded @@ -161,6 +165,11 @@ public: // virtual dtor for any base class virtual ~wxMBConv() { } + +private: + // Common part of single argument cWC2MB() and cMB2WC() overloads above. + wxCharBuffer DoConvertWC2MB(const wchar_t* pwz, size_t srcLen) const; + wxWCharBuffer DoConvertMB2WC(const char* psz, size_t srcLen) const; }; // ---------------------------------------------------------------------------- diff --git a/src/common/strconv.cpp b/src/common/strconv.cpp index 94ff70d1b0..11d1adbc54 100644 --- a/src/common/strconv.cpp +++ b/src/common/strconv.cpp @@ -394,42 +394,6 @@ size_t wxMBConv::WC2MB(char *outBuff, const wchar_t *inBuff, size_t outLen) cons return rc; } -wxWCharBuffer wxMBConv::cMB2WC(const char *psz) const -{ - if ( psz ) - { - // calculate the length of the buffer needed first - const size_t nLen = ToWChar(NULL, 0, psz); - if ( nLen != wxCONV_FAILED ) - { - // now do the actual conversion - wxWCharBuffer buf(nLen - 1 /* +1 added implicitly */); - - // +1 for the trailing NULL - if ( ToWChar(buf.data(), nLen, psz) != wxCONV_FAILED ) - return buf; - } - } - - return wxWCharBuffer(); -} - -wxCharBuffer wxMBConv::cWC2MB(const wchar_t *pwz) const -{ - if ( pwz ) - { - const size_t nLen = FromWChar(NULL, 0, pwz); - if ( nLen != wxCONV_FAILED ) - { - wxCharBuffer buf(nLen - 1); - if ( FromWChar(buf.data(), nLen, pwz) != wxCONV_FAILED ) - return buf; - } - } - - return wxCharBuffer(); -} - wxWCharBuffer wxMBConv::cMB2WC(const char *inBuff, size_t inLen, size_t *outLen) const { @@ -506,10 +470,13 @@ wxMBConv::cWC2MB(const wchar_t *inBuff, size_t inLen, size_t *outLen) const return wxCharBuffer(); } -wxWCharBuffer wxMBConv::cMB2WC(const wxScopedCharBuffer& buf) const +wxWCharBuffer wxMBConv::DoConvertMB2WC(const char* buf, size_t srcLen) const { - const size_t srcLen = buf.length(); - if ( srcLen ) + // Notice that converting NULL pointer should work, i.e. return an empty + // buffer instead of crashing, so we need to check both the length and the + // pointer because length is wxNO_LEN if it's a raw pointer and doesn't + // come from wxScopedCharBuffer. + if ( srcLen && buf ) { const size_t dstLen = ToWChar(NULL, 0, buf, srcLen); if ( dstLen != wxCONV_FAILED ) @@ -517,17 +484,24 @@ wxWCharBuffer wxMBConv::cMB2WC(const wxScopedCharBuffer& buf) const wxWCharBuffer wbuf(dstLen); wbuf.data()[dstLen] = L'\0'; if ( ToWChar(wbuf.data(), dstLen, buf, srcLen) != wxCONV_FAILED ) + { + // If the input string was NUL-terminated, we shouldn't include + // the length of the trailing NUL into the length of the return + // value. + if ( srcLen == wxNO_LEN ) + wbuf.shrink(dstLen - 1); + return wbuf; + } } } - return wxScopedWCharBuffer::CreateNonOwned(L"", 0); + return wxWCharBuffer(); } -wxCharBuffer wxMBConv::cWC2MB(const wxScopedWCharBuffer& wbuf) const +wxCharBuffer wxMBConv::DoConvertWC2MB(const wchar_t* wbuf, size_t srcLen) const { - const size_t srcLen = wbuf.length(); - if ( srcLen ) + if ( srcLen && wbuf ) { const size_t dstLen = FromWChar(NULL, 0, wbuf, srcLen); if ( dstLen != wxCONV_FAILED ) @@ -535,11 +509,18 @@ wxCharBuffer wxMBConv::cWC2MB(const wxScopedWCharBuffer& wbuf) const wxCharBuffer buf(dstLen); buf.data()[dstLen] = '\0'; if ( FromWChar(buf.data(), dstLen, wbuf, srcLen) != wxCONV_FAILED ) + { + // As above, in DoConvertMB2WC(), except that the length of the + // trailing NUL is variable in this case. + if ( srcLen == wxNO_LEN ) + buf.shrink(dstLen - GetMBNulLen()); + return buf; + } } } - return wxScopedCharBuffer::CreateNonOwned("", 0); + return wxCharBuffer(); } // ---------------------------------------------------------------------------- diff --git a/tests/mbconv/mbconvtest.cpp b/tests/mbconv/mbconvtest.cpp index 124a9008a3..0b2b1507c6 100644 --- a/tests/mbconv/mbconvtest.cpp +++ b/tests/mbconv/mbconvtest.cpp @@ -874,6 +874,16 @@ void MBConvTestCase::BufSize() CPPUNIT_ASSERT( convUTF16.WC2MB(buf.data(), utf16text, lenMB + 3) != wxCONV_FAILED ); CPPUNIT_ASSERT_EQUAL( '?', buf[lenMB + 2] ); + + // Test cWC2MB() too. + const wxCharBuffer buf2 = convUTF16.cWC2MB(utf16text); + CHECK( buf2.length() == lenMB ); + CHECK( memcmp(buf, buf2, lenMB) == 0 ); + + const wxWCharBuffer utf16buf = wxWCharBuffer::CreateNonOwned(utf16text); + const wxCharBuffer buf3 = convUTF16.cWC2MB(utf16buf); + CHECK( buf3.length() == lenMB ); + CHECK( memcmp(buf, buf3, lenMB) == 0 ); } void MBConvTestCase::FromWCharTests() @@ -1448,3 +1458,36 @@ void MBConvTestCase::UTF8(const char *charSequence, } #endif // HAVE_WCHAR_H + +TEST_CASE("wxMBConv::cWC2MB", "[mbconv][wc2mb]") +{ + wxMBConvUTF16 convUTF16; + + CHECK( convUTF16.cWC2MB(L"").length() == 0 ); + CHECK( convUTF16.cWC2MB(wxWCharBuffer()).length() == 0 ); + CHECK( convUTF16.cWC2MB(L"Hi").length() == 4 ); + CHECK( convUTF16.cWC2MB(wxWCharBuffer::CreateNonOwned(L"Hi")).length() == 4 ); + + CHECK( wxConvUTF7.cWC2MB(L"").length() == 0 ); + CHECK( wxConvUTF7.cWC2MB(wxWCharBuffer()).length() == 0 ); + CHECK( wxConvUTF7.cWC2MB(L"\xa3").length() == 5 ); + // This test currently fails, the returned value is 3 because the + // conversion object doesn't return to its unshifted state -- which is + // probably a bug in wxMBConvUTF7. + // TODO: fix it there and reenable the test. + CHECK_NOFAIL( wxConvUTF7.cWC2MB(wxWCharBuffer::CreateNonOwned(L"\xa3")).length() == 5 ); +} + +TEST_CASE("wxMBConv::cMB2WC", "[mbconv][mb2wc]") +{ + wxMBConvUTF16 convUTF16; + + CHECK( convUTF16.cMB2WC("\0").length() == 0 ); + CHECK( convUTF16.cMB2WC(wxCharBuffer()).length() == 0 ); + CHECK( convUTF16.cMB2WC("H\0i\0").length() == 2 ); + CHECK( convUTF16.cMB2WC(wxCharBuffer::CreateNonOwned("H\0i\0", 4)).length() == 2 ); + + CHECK( wxConvUTF7.cMB2WC("").length() == 0 ); + CHECK( wxConvUTF7.cMB2WC(wxCharBuffer()).length() == 0 ); + CHECK( wxConvUTF7.cMB2WC("+AKM-").length() == 1 ); +}