Re: A problem with comctl32
DanteAliegri <dantealiegri(a)umbc.edu> wrote:
Hey, I've come across what appears to be a simple problem in comctl32. When running icq99b, wine was dying in imagelist.c while trying to dereference a null pointer. Upon looking at the file, there was code for returning FALSE if that pointer was null, thus I felt it being null may be a valid choice. I made the attached change, and the problem was fixed. Comments?
--- imagelist.c 23 Oct 2002 22:19:11 -0000 1.65 +++ imagelist.c 2 Nov 2002 20:40:53 -0000 @@ -1082,11 +1082,14 @@ HBITMAP hImageBmp, hOldImageBmp, hOldImageListBmp, hOldMaskListBmp, hBlendMaskBmp; BOOL bIsTransparent, bBlend, bResult = FALSE; const HIMAGELIST himl = pimldp->himl; ^^^^^^^^^^^^ According to the same lines pimldp could also be NULL so this might as well cause a NULL pointer dereference and should be moved to after the check for "if (!pimldp || !(himl = pimldp->himl)) return FALSE;
Of course the question remains why would you call the function at all with a NULL pointer.
- const INT lx = himl->cx * pimldp->i + pimldp->xBitmap; - const INT ly = pimldp->yBitmap; + static INT lx; + static INT ly;
Should this be really static? Can't this function be called reentrant?
if (!pimldp || !himl) return FALSE; if ((pimldp->i < 0) || (pimldp->i >= himl->cCurImage)) return FALSE; + + lx = himl->cx * pimldp->i + pimldp->xBitmap; + ly = pimldp->yBitmap;
Rolf Kalbermatter
Rolf Kalbermatter wrote:
DanteAliegri <dantealiegri(a)umbc.edu> wrote:
Hey, I've come across what appears to be a simple problem in comctl32. When running icq99b, wine was dying in imagelist.c while trying to dereference a null pointer. Upon looking at the file, there was code for returning FALSE if that pointer was null, thus I felt it being null may be a valid choice. I made the attached change, and the problem was fixed. Comments?
--- imagelist.c 23 Oct 2002 22:19:11 -0000 1.65 +++ imagelist.c 2 Nov 2002 20:40:53 -0000 @@ -1082,11 +1082,14 @@ HBITMAP hImageBmp, hOldImageBmp, hOldImageListBmp, hOldMaskListBmp, hBlendMaskBmp; BOOL bIsTransparent, bBlend, bResult = FALSE; const HIMAGELIST himl = pimldp->himl;
^^^^^^^^^^^^ According to the same lines pimldp could also be NULL so this might as well cause a NULL pointer dereference and should be moved to after the check for "if (!pimldp || !(himl = pimldp->himl)) return FALSE;
Of course the question remains why would you call the function at all with a NULL pointer.
That is a good question. I didn't do any looking into this; however the function that this is in is BOOL WINAPI ImageList_DrawIndirect (IMAGELISTDRAWPARAMS *pimldp) So pimldp and himl aren't handed to that function. I was thinking that it had something do to with asking it to draw something that you hadn't given the images for yet ( so some program that just does while( 1 ) { draw() ; maybe_add_a_picture(); } ) as some way to make best response, or something silly like that.
- const INT lx = himl->cx * pimldp->i + pimldp->xBitmap; - const INT ly = pimldp->yBitmap; + static INT lx; + static INT ly;
Should this be really static? Can't this function be called reentrant?
well, static is no worse than const ;) but I'll leave that to dimitrie. I was simply trying to cause as few changes as possible, and changing it to static would make it a global , rather than a stack variable, the same as const. -Dante
- const INT lx = himl->cx * pimldp->i + pimldp->xBitmap; - const INT ly = pimldp->yBitmap; + static INT lx; + static INT ly;
Should this be really static? Can't this function be called reentrant?
well, static is no worse than const ;)
It is. Your program is no longer threadsafe. Ciao, Marcus
- const INT lx = himl->cx * pimldp->i + pimldp->xBitmap; - const INT ly = pimldp->yBitmap; + static INT lx; + static INT ly;
Should this be really static? Can't this function be called reentrant?
well, static is no worse than const ;) but I'll leave that to dimitrie. I was simply trying to cause as few changes as possible, and changing it to static would make it a global , rather than a stack variable, the same as const.
There is a huge distinction between static and const. static is allocated on the heap as global as you say and that means bad things will happen when the function is called in parallel from several threads. const basically is only a compiler restriction which prevents the programmer to change the value of a variable after it is initialized. That's why you had to remove it as otherwise the compiler complained that the variable assignement later on would not be valid. But the const variable is still allocated on the stack and therefore threadsafe. Rolf Kalbermatter
Rolf Kalbermatter wrote:
There is a huge distinction between static and const. static is allocated on the heap as global as you say and that means bad things will happen when the function is called in parallel from several threads.
const basically is only a compiler restriction which prevents the programmer to change the value of a variable after it is initialized. That's why you had to remove it as otherwise the compiler complained that the variable assignement later on would not be valid. But the const variable is still allocated on the stack and therefore threadsafe.
Rolf Kalbermatter
Ah, yes, I know what const and static do. I was just thinking that const was a global as well as static .... I must be thinking C++/constructors and be getting confused. So just plain INT is is.
On November 4, 2002 07:33 am, DanteAliegri wrote:
- const INT lx = himl->cx * pimldp->i + pimldp->xBitmap; - const INT ly = pimldp->yBitmap; + static INT lx; + static INT ly;
Should this be really static? Can't this function be called reentrant?
well, static is no worse than const ;) but I'll leave that to dimitrie. I was simply trying to cause as few changes as possible, and changing it to static would make it a global , rather than a stack variable, the same as const.
No, static is very, very different from const. I didn't even see that you've made that change. As it is, the patch's no good. Just drop the static. -- Dimi.
participants (5)
-
DanteAliegri -
Dimitrie O. Paun -
Marcus Meissner -
Rolf Kalbermatter -
Rolf Kalbermatter