Re: gdi32 : Correction of bug 20850 Write buffer overflow in WidenPath()
On Sat, Nov 28, 2009 at 5:04 PM, Laurent Vromman <laurent(a)vromman.org> wrote:
- pStrokes = HeapAlloc(GetProcessHeap(), 0, numStrokes * sizeof(GdiPath*)); - pStrokes[0] = HeapAlloc(GetProcessHeap(), 0, sizeof(GdiPath)); - PATH_InitGdiPath(pStrokes[0]); - pStrokes[0]->pFlags = HeapAlloc(GetProcessHeap(), 0, pPath->numEntriesUsed * sizeof(INT)); - pStrokes[0]->pPoints = HeapAlloc(GetProcessHeap(), 0, pPath->numEntriesUsed * sizeof(POINT)); - pStrokes[0]->numEntriesUsed = 0; + pStrokes = NULL; ...
Wait, wouldn't it be simpler to keep the initial alloc, as in the attachment? - Dan
On Sun, Nov 29, 2009 at 2:32 AM, Dan Kegel <dank(a)kegel.com> wrote:
On Sat, Nov 28, 2009 at 5:04 PM, Laurent Vromman <laurent(a)vromman.org> wrote:
- pStrokes = HeapAlloc(GetProcessHeap(), 0, numStrokes * sizeof(GdiPath*)); - pStrokes[0] = HeapAlloc(GetProcessHeap(), 0, sizeof(GdiPath)); - PATH_InitGdiPath(pStrokes[0]); - pStrokes[0]->pFlags = HeapAlloc(GetProcessHeap(), 0, pPath->numEntriesUsed * sizeof(INT)); - pStrokes[0]->pPoints = HeapAlloc(GetProcessHeap(), 0, pPath->numEntriesUsed * sizeof(POINT)); - pStrokes[0]->numEntriesUsed = 0; + pStrokes = NULL; ...
Wait, wouldn't it be simpler to keep the initial alloc, as in the attachment?
Yes, it would be. The fact is that makes a zero byte size HeapAlloc. I'm not really sure how and why it is supposed to work in this case, so I prefered to removed it. MSDN didn't help me to understand what is a pointer to a zero byte size memory area, and how HeapReAlloc is supposed to react then. If you say it's ok, you're solution is way better. pStrokes = HeapAlloc(GetProcessHeap(), 0, numStrokes * sizeof(GdiPath*)); can even be replaced by pStrokes = HeapAlloc(GetProcessHeap(), 0, 0); It's useless to call sizeof if numStrokes is 0. If you are ok with all that, I can make a new patch based on your proposal.
- Dan
Laurent
Laurent wrote:
On Sun, Nov 29, 2009 at 2:32 AM, Dan Kegel <dank(a)kegel.com> wrote:
On Sat, Nov 28, 2009 at 5:04 PM, Laurent Vromman <laurent(a)vromman.org> wrote:
- pStrokes = HeapAlloc(GetProcessHeap(), 0, numStrokes * sizeof(GdiPath*)); - pStrokes[0] = HeapAlloc(GetProcessHeap(), 0, sizeof(GdiPath)); - PATH_InitGdiPath(pStrokes[0]); - pStrokes[0]->pFlags = HeapAlloc(GetProcessHeap(), 0, pPath->numEntriesUsed * sizeof(INT)); - pStrokes[0]->pPoints = HeapAlloc(GetProcessHeap(), 0, pPath->numEntriesUsed * sizeof(POINT)); - pStrokes[0]->numEntriesUsed = 0; + pStrokes = NULL;
...
Wait, wouldn't it be simpler to keep the initial alloc, as in the attachment?
Yes, it would be. The fact is that makes a zero byte size HeapAlloc. I'm not really sure how and why it is supposed to work in this case, so I prefered to removed it. MSDN didn't help me to understand what is a pointer to a zero byte size memory area, and how HeapReAlloc is supposed to react then.
If you say it's ok, you're solution is way better.
pStrokes = HeapAlloc(GetProcessHeap(), 0, numStrokes * sizeof(GdiPath*)); can even be replaced by pStrokes = HeapAlloc(GetProcessHeap(), 0, 0); I think it will be optimized automatically to that, but call it such way is confusing, if it's necessary to allocate a zero sized area to make next HeapReAlloc() succeed let's specify it explicitly. It's useless to call sizeof if numStrokes is 0.
It's not a call, it's compile time calculated.
If you are ok with all that, I can make a new patch based on your proposal.
- Dan
Laurent
On Sun, Nov 29, 2009 at 12:10 AM, Laurent <laurent(a)vromman.org> wrote:
Yes, it would be. The fact is that makes a zero byte size HeapAlloc. I'm not really sure how and why it is supposed to work in this case,
It works fine, HeapReAlloc handles it ok, too. On Windows, it actually returns a heap block. (On Posix, it can return null, but realloc still works either way.) Really, one would probably prefer to reduce the number of reallocs by allocating extra entries initially (say, four) and then doubling each time that wasn't enough, but for now let's go with the simplest possible change that will make your code correct. - Dan
On Sun, Nov 29, 2009 at 2:24 PM, Dan Kegel <dank(a)kegel.com> wrote:
On Sun, Nov 29, 2009 at 12:10 AM, Laurent <laurent(a)vromman.org> wrote:
Yes, it would be. The fact is that makes a zero byte size HeapAlloc. I'm not really sure how and why it is supposed to work in this case,
It works fine, HeapReAlloc handles it ok, too. On Windows, it actually returns a heap block. (On Posix, it can return null, but realloc still works either way.)
Thank you for the precision. Let apply your patch in this case. It's not necessary that I reproduce the same with the name on it as the only difference.
Really, one would probably prefer to reduce the number of reallocs by allocating extra entries initially (say, four) and then doubling each time that wasn't enough, but for now let's go with the simplest possible change that will make your code correct. - Dan
Laurent
participants (3)
-
Dan Kegel -
Laurent -
Nikolay Sivov