Alexey wrote:
i currently digg in comctl32 to find why my app fails. I found that string conversation AtoW in some places silently fails. The problem is that the destination for string is just a fresh pointer (not NULL). Str_SetPtrAtoW check if it is NULL pointer and if not it trys to ReAlloc. There was no Alloc before so ReAlloc returns NULL
The code looks like it assumes that pointer is always managed by Alloc/ReAlloc/Free. In what context is the destination a fresh, non-Alloc'd pointer? Perhaps that's where the bug lies.
Your proposed solution would pass a non-Alloc'd pointer to Free, which doesn't seem good. - Dan
Am Samstag, den 30.04.2011, 14:41 +0000 schrieb Dan Kegel:
Alexey wrote:
i currently digg in comctl32 to find why my app fails. I found that string conversation AtoW in some places silently fails. The problem is that the destination for string is just a fresh pointer (not NULL). Str_SetPtrAtoW check if it is NULL pointer and if not it trys to ReAlloc. There was no Alloc before so ReAlloc returns NULL
The code looks like it assumes that pointer is always managed by Alloc/ReAlloc/Free. In what context is the destination a fresh, non-Alloc'd pointer? Perhaps that's where the bug lies.
We do not need to save anything what was in the target. Even the old code ReAloocate it and then rewrite. In most contexts the pointer is a fresh one. If it is not, it should be freed before this func.
Your proposed solution would pass a non-Alloc'd pointer to Free, which doesn't seem good.
So probably best solution is to forbid using not a NULL pointer in this func. The programmer should decide where to Free it before.
On So, 2011-05-01 at 07:43 +0200, Alexey Fisher wrote:
Am Samstag, den 30.04.2011, 14:41 +0000 schrieb Dan Kegel:
Alexey wrote:
i currently digg in comctl32 to find why my app fails. I found that string conversation AtoW in some places silently fails. The problem is that the destination for string is just a fresh pointer (not NULL). Str_SetPtrAtoW check if it is NULL pointer and if not it trys to ReAlloc. There was no Alloc before so ReAlloc returns NULL
The code looks like it assumes that pointer is always managed by Alloc/ReAlloc/Free. In what context is the destination a fresh, non-Alloc'd pointer? Perhaps that's where the bug lies.
We do not need to save anything what was in the target. Even the old code ReAloocate it and then rewrite. In most contexts the pointer is a fresh one. If it is not, it should be freed before this func.
Your proposed solution would pass a non-Alloc'd pointer to Free, which doesn't seem good.
So probably best solution is to forbid using not a NULL pointer in this func. The programmer should decide where to Free it before.
There is a new patch in the attachment.
On Sun, May 1, 2011 at 2:07 AM, Alexey Fisher bug-track@fisher-privat.net wrote:
The code looks like it assumes that pointer is always managed by Alloc/ReAlloc/Free. In what context is the destination a fresh, non-Alloc'd pointer? Perhaps that's where the bug lies.
We do not need to save anything what was in the target. Even the old code ReAloocate it and then rewrite. In most contexts the pointer is a fresh one. If it is not, it should be freed before this func.
I don't see why we should need to change the contract so much. Re-using existing blocks of memory isn't the worst idea in the world.
There is a new patch in the attachment.
It seems there's still some mystery in the code, and that you don't fully understand it, so your patch probably isn't right. - Dan
On So, 2011-05-01 at 08:05 -0700, Dan Kegel wrote:
On Sun, May 1, 2011 at 2:07 AM, Alexey Fisher bug-track@fisher-privat.net wrote:
The code looks like it assumes that pointer is always managed by Alloc/ReAlloc/Free. In what context is the destination a fresh, non-Alloc'd pointer? Perhaps that's where the bug lies.
We do not need to save anything what was in the target. Even the old code ReAloocate it and then rewrite. In most contexts the pointer is a fresh one. If it is not, it should be freed before this func.
I don't see why we should need to change the contract so much. Re-using existing blocks of memory isn't the worst idea in the world.
Probably not, but: Correct me if I'm wrong. You can't ReAlloc pointer only if you allocated the memory by Alloc. At least for glibc.
There is a new patch in the attachment.
It seems there's still some mystery in the code, and that you don't fully understand it, so your patch probably isn't right.
you right.
I'll continue to digg and send new patch ASAP.
On 05/01/2011 10:29 AM, Alexey Fisher wrote:
Correct me if I'm wrong. You can't ReAlloc pointer only if you allocated the memory by Alloc. At least for glibc.
First of all we talking about Wine's functions HeapAlloc, HeapReAlloc, and HeapFree.
The HeapReAlloc can and does re-allocate memory initially allocated by HeapAlloc. However it can not allocate memory first time around - calling HeapReAlloc with null src pointer is an error.
What Dan was talking about - HeapReAlloc will attempt to reuse the same memory if new size is same or smaller then current size. This can significantly improve performance.
Vitaliy.
On 5/1/2011 13:07, Alexey Fisher wrote:
There is a new patch in the attachment.
BOOL Str_SetPtrAtoW (LPWSTR *lppDest, LPCSTR lpSrc) {
- TRACE("(%p %s)\n", lppDest, lpSrc);
- TRACE("(%p, %s)\n", *lppDest, debugstr_a(lpSrc));
- if (*lppDest) {
ERR("lppDest should be NULL!");return FALSE;- }
It's an internal call, so it's better to require a caller to pass valid parameters. That's why ERR is too much here, cause you completely control passed parameters in a first place.
Please open a bug with a test C application so we can look at initial problem.
On So, 2011-05-01 at 19:11 +0400, Nikolay Sivov wrote:
On 5/1/2011 13:07, Alexey Fisher wrote:
There is a new patch in the attachment.
BOOL Str_SetPtrAtoW (LPWSTR *lppDest, LPCSTR lpSrc) {
- TRACE("(%p %s)\n", lppDest, lpSrc);
- TRACE("(%p, %s)\n", *lppDest, debugstr_a(lpSrc));
- if (*lppDest) {
ERR("lppDest should be NULL!");return FALSE;- }
It's an internal call, so it's better to require a caller to pass valid parameters. That's why ERR is too much here, cause you completely control passed parameters in a first place.
The problem is, this function return FALSE if some thing going wrong, but caller never check it. So if we pass fresh not NULL, not Alloc'd pointer. We have some garbage in pointers target before the call. After ReAlloc and Str_SetPtrAtoW failed, caller continues to use old pointers target, also old garbage. The App get garbage instead of the string and will crash in some conditions. This all make it hard to find the cause of the crash.
Please open a bug with a test C application so we can look at initial problem.
Das Not ReAllocing not Alloc'd memory is bug? or it is future?
Here is part of man realloc:
realloc() changes the size of the memory block pointed to by ptr to size bytes. The contents will be unchanged to the minimum of the old and new sizes; newly allocated memory will be uninitialized. If ptr is NULL, then the call is equivalent to malloc(size), for all values of size; if size is equal to zero, and ptr is not NULL, then the call is equivalent to free(ptr). Unless ptr is NULL, it must have been returned by an earlier call to malloc(), calloc() or realloc(). If the area pointed to was moved, a free(ptr) is done.
On 5/1/2011 21:49, Alexey Fisher wrote:
On So, 2011-05-01 at 19:11 +0400, Nikolay Sivov wrote:
On 5/1/2011 13:07, Alexey Fisher wrote:
There is a new patch in the attachment. BOOL Str_SetPtrAtoW (LPWSTR *lppDest, LPCSTR lpSrc) {
- TRACE("(%p %s)\n", lppDest, lpSrc);
- TRACE("(%p, %s)\n", *lppDest, debugstr_a(lpSrc));
- if (*lppDest) {
ERR("lppDest should be NULL!");return FALSE;- }
It's an internal call, so it's better to require a caller to pass valid parameters. That's why ERR is too much here, cause you completely control passed parameters in a first place.
The problem is, this function return FALSE if some thing going wrong, but caller never check it. So if we pass fresh not NULL, not Alloc'd pointer. We have some garbage in pointers target before the call. After ReAlloc and Str_SetPtrAtoW failed, caller continues to use old pointers target, also old garbage. The App get garbage instead of the string and will crash in some conditions. This all make it hard to find the cause of the crash.
I'll try to explain again.
Str_SetPtrAtoW() is not a public API call, so it's not accessible directly from outside comctl32. My request to you was to provide a simple test application that uses some comctl32 controls presumably and indirectly makes use of this call (and fails for some reason you'll trying to find).
Please open a bug with a test C application so we can look at initial problem.
Das Not ReAllocing not Alloc'd memory is bug? or it is future?
It depends.
Here is part of man realloc:
realloc() changes the size of the memory block pointed to by ptr to size bytes. The contents will be unchanged to the minimum of the old and new sizes; newly allocated memory will be uninitialized. If ptr is NULL, then the call is equivalent to malloc(size), for all values of size; if size is equal to zero, and ptr is not NULL, then the call is equivalent to free(ptr). Unless ptr is NULL, it must have been returned by an earlier call to malloc(), calloc() or realloc(). If the area pointed to was moved, a free(ptr) is done.
This is irrelevant here.
On So, 2011-05-01 at 21:55 +0400, Nikolay Sivov wrote:
On 5/1/2011 21:49, Alexey Fisher wrote:
On So, 2011-05-01 at 19:11 +0400, Nikolay Sivov wrote:
On 5/1/2011 13:07, Alexey Fisher wrote:
There is a new patch in the attachment. BOOL Str_SetPtrAtoW (LPWSTR *lppDest, LPCSTR lpSrc) {
- TRACE("(%p %s)\n", lppDest, lpSrc);
- TRACE("(%p, %s)\n", *lppDest, debugstr_a(lpSrc));
- if (*lppDest) {
ERR("lppDest should be NULL!");return FALSE;- }
It's an internal call, so it's better to require a caller to pass valid parameters. That's why ERR is too much here, cause you completely control passed parameters in a first place.
The problem is, this function return FALSE if some thing going wrong, but caller never check it. So if we pass fresh not NULL, not Alloc'd pointer. We have some garbage in pointers target before the call. After ReAlloc and Str_SetPtrAtoW failed, caller continues to use old pointers target, also old garbage. The App get garbage instead of the string and will crash in some conditions. This all make it hard to find the cause of the crash.
I'll try to explain again.
Str_SetPtrAtoW() is not a public API call, so it's not accessible directly from outside comctl32. My request to you was to provide a simple test application that uses some comctl32 controls presumably and indirectly makes use of this call (and fails for some reason you'll trying to find).
Please open a bug with a test C application so we can look at initial problem.
Das Not ReAllocing not Alloc'd memory is bug? or it is future?
It depends.
Here is part of man realloc:
realloc() changes the size of the memory block pointed to by ptr to size bytes. The contents will be unchanged to the minimum of the old and new sizes; newly allocated memory will be uninitialized. If ptr is NULL, then the call is equivalent to malloc(size), for all values of size; if size is equal to zero, and ptr is not NULL, then the call is equivalent to free(ptr). Unless ptr is NULL, it must have been returned by an earlier call to malloc(), calloc() or realloc(). If the area pointed to was moved, a free(ptr) is done.
This is irrelevant here.
I need to apologize. Wine a big mystery for me. And it seems like the string corruption i talked about, i introduced by my self. And i see you send the patch to test the strings: comctl32/tests: Cleanup MRU lists tests, add more
At least i got the target and found how to workaround my bug http://bugs.winehq.org/show_bug.cgi?id=26923 :D
-
Alexey Fisher -
Dan Kegel -
Nikolay Sivov -
Vitaliy Margolen