On Saturday, 23 April 2016 3:35 AM, Sebastian Lackner wrote:
The current calculation does not take the terminating \0 into account.
-buffer = HeapAlloc(GetProcessHeap(), 0, tmp_size * 2); +buffer = HeapAlloc(GetProcessHeap(), 0, tmp_size * 2 + sizeof(WCHAR));
Which particular cases cause the heap corruption?
-- Hugh McMaster
On 23.04.2016 11:56, Hugh McMaster wrote:
On Saturday, 23 April 2016 3:35 AM, Sebastian Lackner wrote:
The current calculation does not take the terminating \0 into account.
-buffer = HeapAlloc(GetProcessHeap(), 0, tmp_size * 2); +buffer = HeapAlloc(GetProcessHeap(), 0, tmp_size * 2 + sizeof(WCHAR));
Which particular cases cause the heap corruption?
-- Hugh McMaster
It happens for example with a string like L"\0\0\0\0". After stripping off the last two "\0" the allocated memory will have a size of 4 characters, but the code below writes 5 characters. You can also test with warn+heap after creating such a registry key.
On Saturday, 23 April 2016 10:24 PM, Sebastian Lackner wrote:
On 23.04.2016 11:56, Hugh McMaster wrote:
On Saturday, 23 April 2016 3:35 AM, Sebastian Lackner wrote:
The current calculation does not take the terminating \0 into account.
-buffer = HeapAlloc(GetProcessHeap(), 0, tmp_size * 2); +buffer = HeapAlloc(GetProcessHeap(), 0, tmp_size * 2 + sizeof(WCHAR));
Which particular cases cause the heap corruption?
It happens for example with a string like L"\0\0\0\0". After stripping off the last two "\0" the allocated memory will have a size of 4 characters, but the code below writes 5 characters. You can also test with warn+heap after creating such a registry key.
Such a string is invalid on Windows. You can't add it using regedit (the string is trimmed to a single \0 after closing the dialog editor). It also causes an error with 'reg add'.
That said, users could potentially modify Wine's user.reg, causing a heap corruption, so this patch is fine. I'll send a Sign-Off.
On 24.04.2016 14:53, Hugh McMaster wrote:
On Saturday, 23 April 2016 10:24 PM, Sebastian Lackner wrote:
On 23.04.2016 11:56, Hugh McMaster wrote:
On Saturday, 23 April 2016 3:35 AM, Sebastian Lackner wrote:
The current calculation does not take the terminating \0 into account.
-buffer = HeapAlloc(GetProcessHeap(), 0, tmp_size * 2); +buffer = HeapAlloc(GetProcessHeap(), 0, tmp_size * 2 + sizeof(WCHAR));
Which particular cases cause the heap corruption?
It happens for example with a string like L"\0\0\0\0". After stripping off the last two "\0" the allocated memory will have a size of 4 characters, but the code below writes 5 characters. You can also test with warn+heap after creating such a registry key.
Such a string is invalid on Windows. You can't add it using regedit (the string is trimmed to a single \0 after closing the dialog editor). It also causes an error with 'reg add'.
That said, users could potentially modify Wine's user.reg, causing a heap corruption, so this patch is fine. I'll send a Sign-Off.
I've reproduced it by using regedit, so its not just a theoretical issue.
On Sunday, 24 April 2016 10:54 PM, Sebastian Lackner wrote:
On 24.04.2016 14:53, Hugh McMaster wrote:
On Saturday, 23 April 2016 10:24 PM, Sebastian Lackner wrote:
On 23.04.2016 11:56, Hugh McMaster wrote:
Which particular cases cause the heap corruption?
It happens for example with a string like L"\0\0\0\0". After stripping off the last two "\0" the allocated memory will have a size of 4 characters, but the code below writes 5 characters. You can also test with warn+heap after creating such a registry key.
Such a string is invalid on Windows. You can't add it using regedit (the string is trimmed to a single \0 after closing the dialog editor). It also causes an error with 'reg add'.
I've reproduced it by using regedit, so its not just a theoretical issue.
Okay, I worked this out. It is possible to create such a string by using the "Modify binary data" menu option. I couldn't create it using the normal dialog editor, as it trims the string back to a single \0.
Anyway, this is definitely a bug in reg.exe. Thanks for fixing this.
-
Hugh McMaster -
Sebastian Lackner