Re: [PATCH v2 2/3] winex11.drv: avoid passing NULL to memcpy (clang)
Kirill Smirnov <kirill.k.smirnov(a)gmail.com> writes:
@@ -92,7 +92,8 @@ static void X11DRV_ImmSetInternalString(DWORD dwOffset, ptr_new = CompositionString + byte_offset; memmove(ptr_new + byte_length, ptr_new + byte_selection, dwCompStringLength - byte_offset - byte_selection); - memcpy(ptr_new, lpComp, byte_length); + /* Do not pass NULL even if the length == 0 */ + if (lpComp) memcpy(ptr_new, lpComp, byte_length); dwCompStringLength += byte_expansion;
Why would that be a problem? -- Alexandre Julliard julliard(a)winehq.org
According to the standard, memcpy() and its friends should never accept NULL as argument, otherwise the behavior is undefined and optimizer takes this into account. Below in the same file (line 206) caller passes NULL as lpComp argument triggering this case. This patch is of the same nature as the following: commit 5a3ad0ecf08c00fc0b7b2f24e55be27b7c3f5da0 Author: Kirill Smirnov <kirill.k.smirnov(a)gmail.com> Date: Mon May 9 18:56:14 2016 +0300 winhlp32: Do not pass NULL to strchr() (spotted by clang). Signed-off-by: Kirill K. Smirnov <kirill.k.smirnov(a)gmail.com> Signed-off-by: Alexandre Julliard <julliard(a)winehq.org> On 08.07.2016 06:16, Alexandre Julliard wrote:
Kirill Smirnov <kirill.k.smirnov(a)gmail.com> writes:
@@ -92,7 +92,8 @@ static void X11DRV_ImmSetInternalString(DWORD dwOffset, ptr_new = CompositionString + byte_offset; memmove(ptr_new + byte_length, ptr_new + byte_selection, dwCompStringLength - byte_offset - byte_selection); - memcpy(ptr_new, lpComp, byte_length); + /* Do not pass NULL even if the length == 0 */ + if (lpComp) memcpy(ptr_new, lpComp, byte_length); dwCompStringLength += byte_expansion;
Why would that be a problem?
Kirill Smirnov <kirill.k.smirnov(a)gmail.com> writes:
According to the standard, memcpy() and its friends should never accept NULL as argument, otherwise the behavior is undefined and optimizer takes this into account.
Below in the same file (line 206) caller passes NULL as lpComp argument triggering this case.
This patch is of the same nature as the following:
commit 5a3ad0ecf08c00fc0b7b2f24e55be27b7c3f5da0 Author: Kirill Smirnov <kirill.k.smirnov(a)gmail.com> Date: Mon May 9 18:56:14 2016 +0300
winhlp32: Do not pass NULL to strchr() (spotted by clang).
strchr has to access the pointer so obviously it can't take NULL, but memcpy with a zero length shouldn't, so it's not very useful compiler behavior IMO. Sadly it seems the standard does allow it... -- Alexandre Julliard julliard(a)winehq.org
2016-07-09 8:42 GMT+02:00 Alexandre Julliard <julliard(a)winehq.org>:
Kirill Smirnov <kirill.k.smirnov(a)gmail.com> writes:
According to the standard, memcpy() and its friends should never accept NULL as argument, otherwise the behavior is undefined and optimizer takes this into account.
Below in the same file (line 206) caller passes NULL as lpComp argument triggering this case.
This patch is of the same nature as the following:
commit 5a3ad0ecf08c00fc0b7b2f24e55be27b7c3f5da0 Author: Kirill Smirnov <kirill.k.smirnov(a)gmail.com> Date: Mon May 9 18:56:14 2016 +0300
winhlp32: Do not pass NULL to strchr() (spotted by clang).
strchr has to access the pointer so obviously it can't take NULL, but memcpy with a zero length shouldn't, so it's not very useful compiler behavior IMO. Sadly it seems the standard does allow it...
Yeah and gcc has also been exploiting that for more aggressive optimizations for a while (see https://gcc.gnu.org/gcc-4.9/porting_to.html). deb274226783ab886bdb44876944e156757efe2b was probably the one significant precedent for Wine.
participants (3)
-
Alexandre Julliard -
Kirill Smirnov -
Matteo Bruni