Menu
▾
▴
#405 Copy details to clipboard can crash
1) The OpenClipboard call needs a valid window handle.
2) Lines that are longer than the size of the variable
g_tmp causes the the buffer to overflow since on the
second loop the 'cchTextMax' variable is set to 'global'.
3) The buffer is not freed at the end. I added a call
GlobalFree.
This bug is found in 2.09.
//>>>Ximon Eighteen aka Sunjammer 30th August
2002
//+++Popup "Copy Details To Clipboard" menu when
RMB clicked in DetailView
if (uMsg == WM_CONTEXTMENU && wParam ==
(WPARAM) linsthwnd)
{
int count = ListView_GetItemCount(linsthwnd);
if (count > 0)
{
HMENU menu = CreatePopupMenu();
POINT pt;
AppendMenu(menu,MF_STRING,1,GetNSISStringTT
(LANG_COPYDETAILS));
if (lParam == ((UINT)-1))
{
RECT r;
GetWindowRect(linsthwnd, &r);
pt.x = r.left;
pt.y = r.top;
}
else
{
pt.x = GET_X_LPARAM(lParam);
pt.y = GET_Y_LPARAM(lParam);
}
if (1==TrackPopupMenu(
menu,
TPM_NONOTIFY|TPM_RETURNCMD,
pt.x,
pt.y,
0,linsthwnd,0))
{
int i,total = 1; // 1 for the null char
LVITEM item;
HGLOBAL memory;
LPTSTR ptr;//,endPtr;
BOOL bResult;
// 1st pass - determine clipboard memory required.
item.iSubItem = 0;
item.pszText = g_tmp;
item.cchTextMax = sizeof(g_tmp) - 1;
i = count;
while (i--)
// Add 2 for the CR/LF combination that must
follow every line.
total += 2+SendMessage
(linsthwnd,LVM_GETITEMTEXT,i,(LPARAM)&item);
// 2nd pass - store detail view strings on the
clipboard
// Clipboard MSDN docs say
mem must be GMEM_MOVEABLE
bResult = OpenClipboard
(linsthwnd);
//ASSERT(bResult);
bResult = EmptyClipboard();
//ASSERT(bResult);
memory = GlobalAlloc
(GHND,total);
ptr = GlobalLock(memory);
//endPtr = ptr+total-2; // -2 to allow for CR/LF
i = 0;
do {
item.pszText = ptr;
item.cchTextMax = sizeof(g_tmp) - 1;
SendMessage(linsthwnd,LVM_GETITEMTEXT,i,
(LPARAM)&item);
ptr += mystrlen(ptr);
*(WORD*)ptr = CHAR2_TO_WORD('\r','\n');
ptr+=2;
} while (++i < count);
// memory is auto zeroed when
allocated with GHND - *ptr = 0;
bResult = !GlobalUnlock(memory)
&& GetLastError() == NO_ERROR;
//ASSERT(bResult);
bResult = SetClipboardData
(CF_TEXT,memory) != NULL;
//ASSERT(bResult);
bResult = CloseClipboard();
//ASSERT(bResult);
bResult = GlobalFree(memory)
== NULL;
//ASSERT(bResult);
}
}
return FALSE;
}
//<<<
Logged In: YES
user_id=584402
1) OpenClipboard doesn't need a valid window handle. From my
experience, it's also better to pass NULL rather than a
valid window handle. It sometime fails without any good
reason when using a valid handle. From MSDN:
"Handle to the window to be associated with the open
clipboard. If this parameter is NULL, the open clipboard is
associated with the current task."
2) cchTextMax might be set to total, but pszText is also set
to the newly allocated buffer which has enough bytes.
3) That's arguably the correct thing to do. I've read what
MSDN has to say about this again and it can be interpreted
to support both. The examples in MSDN also support not
freeing the, now system-owned, memory object.
==
Why did you start digging into this code? Did an installer
crash on you or did you just happen to stumble upon it?
Logged In: YES
user_id=584402
I was curious as to what exactly MSDN was saying about no.
3, so I wrote a little test that sets the clipboard many
times, but never frees the memory. It allocates 1mb buffers,
puts a random string in them and sets the clipboard. The
memory usage doesn't change by 1mb after the first call.
This means the memory shouldn't be freed, as done in all of
the examples found in MSDN.
clipboard test
Logged In: YES
user_id=1356798
ad 2) After some investigation I found that this is the real
problem. The size of the g_tmp variable is hard-coded to
4096.
If there are lines longer than this, then the allocated buffer will
be too small and you will get an access violation.
The lines can not become longer than NSIS_MAX_STRLEN
(1024 by default) using the DetailPrint command since it will
wrap the lines.
So you're luck that by default it works. Changing the
NSIS_MAX_STRLEN to a value higher that 4096 and printing
lines a maximum length with induce an access violation.
This could be fixed by changing the declaration to 'static char
g_tmp[4 * NSIS_MAX_STRLEN];'
However I came accross this issue by using
nsexec::ExecToLog which does not wrap the output lines to
NSIS_MAX_STRLEN. So without changing the defaults of
NSIS the CopyToClipboard will crash when lines are longer
than 4096 characters.
I tested this by starting a BAT file which does a 'TYPE
mylonglinedfile.txt'. If you create this file with lines of length
6000 then you will definately have a crash.
ad 3) I agree; it is just that the BoundsChecker complained.
Logged In: YES
user_id=1356798
ad 1)
The call to EmptyClipboard actually fails when the clipboard
has been opened with the NULL parameter. The return value
while debugging on my WindowsXP is FALSE and the
GetLastError() returns ERROR_CLIPBOARD_NOT_OPEN
(=1418).
On the web page
http://msdn.microsoft.com/library/default.asp?url=/library/en-
us/winui/winui/windowsuserinterface/dataexchange/clipboard/
clipboardreference/clipboardfunctions/openclipboard.asp they
mention "If an application calls OpenClipboard with hwnd set
to NULL, EmptyClipboard sets the clipboard owner to NULL;
this causes SetClipboardData to fail." so it is clearly wrong.
Maybe the current code works on older Windows platform.
ad 2) in addition to my comment yesterday:
But each line of the listbox may now add 'global' number of
bytes in the second loop. This is not what was calculated in
the first loop. So the allocation for lines longer than sizeof
(g_tmp)-1 will only be sizeof(g_tmp)-1 while the text retrieve
action will copy a most 'global' bytes. This is of course much
more. So this is clearly a bug.
Logged In: YES
user_id=584402
Ah, I see now what you're talking about. It's the allocated
buffer that can overflow, not g_tmp. I've fixed that by
removing `item.cchTextMax = total;` so it stays as
sizeof(g_tmp) - 1. Thanks.
On both Windows XP SP2 and Windows 98, EmptyClipboard
returns TRUE for me. SetClipboardData also works. In my
older copy of OpenClipboard's documentation, this remark
isn't mentioned. Although EmptyClipboard does mentions it
sets the owner to NULL if OpenClipboard was called with
NULL. But what does that mean? It's already set it NULL, why
should SetClipboardData care if it's NULL after
EmptyClipboard or not? Even more, SetClipboardData does fail
if EmptyClipboard is *not* called.
After EmptyClipboard fails for you, does SetClipboardData
still work?
Please attach patches instead of pasting the code next time.
It makes it much easier to read and easier to apply.
Logged In: YES
user_id=1356798
First of all: sorry it took so long to reply. My person
was the only reason for the delay.
The only reason I dug into this code was the crash. This
has now been resolved in version 2.11. Great!
The other issues are merely cosmetic. I like to verify
each function result. Possibly decorated with a
GetLastError call. BoundChecker does not know the excact
behavior of each and every WIN32 call. So this triggered
us for some investigation on this.
I propose to close this bug since the real problem (crash)
has been fixed.
Thanks for the cooperation.
Logged In: YES
user_id=1356798
This bug has already been fixed in the latest CVS version. You can
get the latest version using NSIS Update or another CVS client.
See the FAQ (go to http://nsis.sf.net and click FAQ) for more
information.