From b4b6d29cc13fff30b2f399327a7047a07b2b536b Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Thu, 25 Jun 2020 10:27:37 +0200 Subject: [PATCH 1/3] Fix #63208: BSTR to PHP string conversion not binary safe A `BSTR` is similar to a `zend_string`; it stores the length of the string just before the actual string, and thus the string may contain NUL bytes. However, `php_com_olestring_to_string()` is supposed to deal with arbitrary `OLECHAR*`s which may not be `BSTR`s, so we introduce `php_com_bstr_to_string()` and use it for the only case where we actually have to deal with `BSTR`s which may contain NUL bytes. Contrary to `php_com_olestring_to_string()` we return a `zend_string`, so we can save the re-allocation when converting to a `zval`. --- ext/com_dotnet/com_olechar.c | 20 ++++++++++++++++++++ ext/com_dotnet/com_variant.c | 8 ++------ ext/com_dotnet/php_com_dotnet_internal.h | 1 + ext/com_dotnet/tests/bug63208.phpt | 18 ++++++++++++++++++ 4 files changed, 41 insertions(+), 6 deletions(-) create mode 100644 ext/com_dotnet/tests/bug63208.phpt diff --git a/ext/com_dotnet/com_olechar.c b/ext/com_dotnet/com_olechar.c index b9a332e4f5442..4f7b9bd0cf395 100644 --- a/ext/com_dotnet/com_olechar.c +++ b/ext/com_dotnet/com_olechar.c @@ -103,3 +103,23 @@ PHP_COM_DOTNET_API char *php_com_olestring_to_string(OLECHAR *olestring, size_t return string; } + +zend_string *php_com_bstr_to_string(BSTR bstr, int codepage) +{ + zend_string *string; + uint32_t length = SysStringLen(bstr); + + string = zend_string_alloc(length, 0); + length = WideCharToMultiByte(codepage, 0, bstr, length + 1, ZSTR_VAL(string), length + 1, NULL, NULL); + + if (length <= 0) { + char *msg = php_win32_error_to_msg(GetLastError()); + + php_error_docref(NULL, E_WARNING, + "Could not convert string from unicode: `%s'", msg); + + LocalFree(msg); + } + + return string; +} diff --git a/ext/com_dotnet/com_variant.c b/ext/com_dotnet/com_variant.c index f4f7d5a9dde3c..22f9697c5c963 100644 --- a/ext/com_dotnet/com_variant.c +++ b/ext/com_dotnet/com_variant.c @@ -236,12 +236,8 @@ PHP_COM_DOTNET_API int php_com_zval_from_variant(zval *z, VARIANT *v, int codepa case VT_BSTR: olestring = V_BSTR(v); if (olestring) { - size_t len; - char *str = php_com_olestring_to_string(olestring, - &len, codepage); - ZVAL_STRINGL(z, str, len); - // TODO: avoid reallocation??? - efree(str); + zend_string *str = php_com_bstr_to_string(olestring, codepage); + ZVAL_STR(z, str); olestring = NULL; } break; diff --git a/ext/com_dotnet/php_com_dotnet_internal.h b/ext/com_dotnet/php_com_dotnet_internal.h index a2fe81368322c..9820023430660 100644 --- a/ext/com_dotnet/php_com_dotnet_internal.h +++ b/ext/com_dotnet/php_com_dotnet_internal.h @@ -89,6 +89,7 @@ PHP_COM_DOTNET_API char *php_com_olestring_to_string(OLECHAR *olestring, size_t *string_len, int codepage); PHP_COM_DOTNET_API OLECHAR *php_com_string_to_olestring(char *string, size_t string_len, int codepage); +zend_string *php_com_bstr_to_string(BSTR bstr, int codepage); /* com_com.c */ diff --git a/ext/com_dotnet/tests/bug63208.phpt b/ext/com_dotnet/tests/bug63208.phpt new file mode 100644 index 0000000000000..426c52cf16d13 --- /dev/null +++ b/ext/com_dotnet/tests/bug63208.phpt @@ -0,0 +1,18 @@ +--TEST-- +Bug #63208 (BSTR to PHP string conversion not binary safe) +--SKIPIF-- + +--FILE-- + +--EXPECT-- +string(10) "6162006364" +string(10) "6162006364" + From 612a877cd5df05e4e2ac0c0e1d47bb8aed8857a3 Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Thu, 25 Jun 2020 16:37:13 +0200 Subject: [PATCH 2/3] Fix up We have to properly determine the output length of the conversion to multi byte strings. We also cater to `php_com_string_to_olestring()` not being binary safe, with basically the same fix we did for `php_com_olestring_to_string()`. --- ext/com_dotnet/com_olechar.c | 53 ++++++++++++++++++++---- ext/com_dotnet/com_variant.c | 9 +--- ext/com_dotnet/php_com_dotnet_internal.h | 1 + ext/com_dotnet/tests/bug63208.phpt | 9 ++-- 4 files changed, 52 insertions(+), 20 deletions(-) diff --git a/ext/com_dotnet/com_olechar.c b/ext/com_dotnet/com_olechar.c index 4f7b9bd0cf395..95ce3cc8ecba9 100644 --- a/ext/com_dotnet/com_olechar.c +++ b/ext/com_dotnet/com_olechar.c @@ -104,21 +104,60 @@ PHP_COM_DOTNET_API char *php_com_olestring_to_string(OLECHAR *olestring, size_t return string; } -zend_string *php_com_bstr_to_string(BSTR bstr, int codepage) +BSTR php_com_string_to_bstr(zend_string *string, int codepage) { - zend_string *string; - uint32_t length = SysStringLen(bstr); + BSTR bstr; + OLECHAR *olestring = NULL; + DWORD flags = codepage == CP_UTF8 ? 0 : MB_PRECOMPOSED | MB_ERR_INVALID_CHARS; + size_t mb_len = ZSTR_LEN(string); + int wc_len; + + wc_len = MultiByteToWideChar(codepage, flags, ZSTR_VAL(string), (int)mb_len + 1, NULL, 0); + if (wc_len > 0) { + olestring = (OLECHAR*)safe_emalloc(wc_len, sizeof(OLECHAR), 0); + wc_len = MultiByteToWideChar(codepage, flags, ZSTR_VAL(string), (int)mb_len + 1, olestring, wc_len); + } else { + char *msg = php_win32_error_to_msg(GetLastError()); + + php_error_docref(NULL, E_WARNING, + "Could not convert string to unicode: `%s'", msg); + LocalFree(msg); + + if (olestring == NULL) { + olestring = (OLECHAR*)emalloc(2 * sizeof(OLECHAR)); + } + *olestring = *(olestring + 1) = '\0'; + } + + bstr = SysAllocStringLen(olestring, (UINT)(wc_len - 1)); + efree(olestring); - string = zend_string_alloc(length, 0); - length = WideCharToMultiByte(codepage, 0, bstr, length + 1, ZSTR_VAL(string), length + 1, NULL, NULL); + return bstr; +} - if (length <= 0) { +zend_string *php_com_bstr_to_string(BSTR bstr, int codepage) +{ + zend_string *string = NULL; + UINT wc_len = SysStringLen(bstr); + int mb_len; + + mb_len = WideCharToMultiByte(codepage, 0, bstr, wc_len + 1, NULL, 0, NULL, NULL); + if (mb_len > 0) { + string = zend_string_alloc(mb_len - 1, 0); + mb_len = WideCharToMultiByte(codepage, 0, bstr, wc_len + 1, ZSTR_VAL(string), mb_len, NULL, NULL); + } + + if (mb_len <= 0) { char *msg = php_win32_error_to_msg(GetLastError()); php_error_docref(NULL, E_WARNING, "Could not convert string from unicode: `%s'", msg); - LocalFree(msg); + + if (string != NULL) { + zend_string_release(string); + } + string = ZSTR_EMPTY_ALLOC(); } return string; diff --git a/ext/com_dotnet/com_variant.c b/ext/com_dotnet/com_variant.c index 22f9697c5c963..901b0a866a17f 100644 --- a/ext/com_dotnet/com_variant.c +++ b/ext/com_dotnet/com_variant.c @@ -96,7 +96,6 @@ static void safe_array_from_zval(VARIANT *v, zval *z, int codepage) PHP_COM_DOTNET_API void php_com_variant_from_zval(VARIANT *v, zval *z, int codepage) { - OLECHAR *olestring; php_com_dotnet_object *obj; zend_uchar ztype = IS_NULL; @@ -164,13 +163,7 @@ PHP_COM_DOTNET_API void php_com_variant_from_zval(VARIANT *v, zval *z, int codep case IS_STRING: V_VT(v) = VT_BSTR; - olestring = php_com_string_to_olestring(Z_STRVAL_P(z), Z_STRLEN_P(z), codepage); - if (CP_UTF8 == codepage) { - V_BSTR(v) = SysAllocStringByteLen((char*)olestring, (UINT)(wcslen(olestring) * sizeof(OLECHAR))); - } else { - V_BSTR(v) = SysAllocStringByteLen((char*)olestring, (UINT)(Z_STRLEN_P(z) * sizeof(OLECHAR))); - } - efree(olestring); + V_BSTR(v) = php_com_string_to_bstr(Z_STR_P(z), codepage); break; case IS_RESOURCE: diff --git a/ext/com_dotnet/php_com_dotnet_internal.h b/ext/com_dotnet/php_com_dotnet_internal.h index 9820023430660..663c0694fe10a 100644 --- a/ext/com_dotnet/php_com_dotnet_internal.h +++ b/ext/com_dotnet/php_com_dotnet_internal.h @@ -89,6 +89,7 @@ PHP_COM_DOTNET_API char *php_com_olestring_to_string(OLECHAR *olestring, size_t *string_len, int codepage); PHP_COM_DOTNET_API OLECHAR *php_com_string_to_olestring(char *string, size_t string_len, int codepage); +BSTR php_com_string_to_bstr(zend_string *string, int codepage); zend_string *php_com_bstr_to_string(BSTR bstr, int codepage); diff --git a/ext/com_dotnet/tests/bug63208.phpt b/ext/com_dotnet/tests/bug63208.phpt index 426c52cf16d13..ae62dbba980c4 100644 --- a/ext/com_dotnet/tests/bug63208.phpt +++ b/ext/com_dotnet/tests/bug63208.phpt @@ -6,13 +6,12 @@ if (!extension_loaded('com_dotnet')) die('skip com_dotnet extension not availabl ?> --FILE-- --EXPECT-- -string(10) "6162006364" -string(10) "6162006364" - +string(14) "e0a48562006364" +string(14) "e0a48562006364" From 3214e3d716d011c3fd7e28aa142a007ad0e91839 Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Mon, 29 Jun 2020 16:42:36 +0200 Subject: [PATCH 3/3] Avoid unnecessary allocation Instead of allocating a temporary `OLECHAR` buffer, we allocate the final `BSTR` right away, and do the multibyte conversion into its buffer. We also refactor to guard statements for better readability. --- ext/com_dotnet/com_olechar.c | 36 ++++++++++++++++-------------------- 1 file changed, 16 insertions(+), 20 deletions(-) diff --git a/ext/com_dotnet/com_olechar.c b/ext/com_dotnet/com_olechar.c index 95ce3cc8ecba9..dd85ed269230f 100644 --- a/ext/com_dotnet/com_olechar.c +++ b/ext/com_dotnet/com_olechar.c @@ -107,32 +107,28 @@ PHP_COM_DOTNET_API char *php_com_olestring_to_string(OLECHAR *olestring, size_t BSTR php_com_string_to_bstr(zend_string *string, int codepage) { BSTR bstr; - OLECHAR *olestring = NULL; DWORD flags = codepage == CP_UTF8 ? 0 : MB_PRECOMPOSED | MB_ERR_INVALID_CHARS; size_t mb_len = ZSTR_LEN(string); int wc_len; - wc_len = MultiByteToWideChar(codepage, flags, ZSTR_VAL(string), (int)mb_len + 1, NULL, 0); - if (wc_len > 0) { - olestring = (OLECHAR*)safe_emalloc(wc_len, sizeof(OLECHAR), 0); - wc_len = MultiByteToWideChar(codepage, flags, ZSTR_VAL(string), (int)mb_len + 1, olestring, wc_len); - } else { - char *msg = php_win32_error_to_msg(GetLastError()); - - php_error_docref(NULL, E_WARNING, - "Could not convert string to unicode: `%s'", msg); - LocalFree(msg); - - if (olestring == NULL) { - olestring = (OLECHAR*)emalloc(2 * sizeof(OLECHAR)); - } - *olestring = *(olestring + 1) = '\0'; + if ((wc_len = MultiByteToWideChar(codepage, flags, ZSTR_VAL(string), (int)mb_len + 1, NULL, 0)) <= 0) { + goto fail; + } + if ((bstr = SysAllocStringLen(NULL, (UINT)(wc_len - 1))) == NULL) { + goto fail; + } + if ((wc_len = MultiByteToWideChar(codepage, flags, ZSTR_VAL(string), (int)mb_len + 1, bstr, wc_len)) <= 0) { + goto fail; } - - bstr = SysAllocStringLen(olestring, (UINT)(wc_len - 1)); - efree(olestring); - return bstr; + +fail: + char *msg = php_win32_error_to_msg(GetLastError()); + php_error_docref(NULL, E_WARNING, + "Could not convert string to unicode: `%s'", msg); + LocalFree(msg); + SysFreeString(bstr); + bstr = SysAllocString(L""); } zend_string *php_com_bstr_to_string(BSTR bstr, int codepage)