Listview bug in comctl32.dll version 6?
Stephen C. Steel
listview control in the new theme-aware comctl32.dll version 6. The problem
only recently appeared when we added a manifest to use themed controls (the
code for this particular dialog hadn't change siginificantly since it was
first written 5 years ago). Removing the manifest from the application
prevents the bug.
1. The application is a C++ MFC bases program compiled as a UNICODE build
using VS .NET 2003. The crash happens in both debug and release builds (and
in a number of versions).
2. The virtual listview control is created by the dialog templace and
associated with CListCtrl member variable using DDXControl(). It is created
with the follwing style bits set: (LVS_REPORT | LVS_NOSORTHEADER |
LVS_SINGLESEL | LVS_SHOWSELALWAYS | LVS_OWNERDATA), and the extended style
bit LVS_EX_FULLROWSELECT is set before any other operations.
3. There are 7 columns defined.
4. Before it crashes, it sends 2 LVN_GETDISPINFO notifications (both for
item 0, subitem 0).
5. It crashes with an access violation with the following call stack:
comctl32.dll!_StringCopyWorkerW@12() + 0x22
comctl32.dll!_CCReturnDispInfoText@12() + 0x2c
comctl32.dll!_ListView_OnGetItem@8() + 0x400
comctl32.dll!_ListView_RGetRects@24() + 0xc5
comctl32.dll!_ListView_RGetRectsEx@20() + 0x23
comctl32.dll!_ListView_RDrawItem@4() + 0x1f7
comctl32.dll!_ListView_DrawItem@4() + 0x225
comctl32.dll!_ListView_Redraw@12() + 0x51e
comctl32.dll!_ListView_OnPaint@8() + 0x1cb
comctl32.dll!_ListView_WndProc@16() + 0x93c
... rest of call stack omitted for brevity - nothing looks wrong
_StringCopyWorkerW@12() is trying to copy a unicode string. The source
address is the LPCTSTR value I used to set the LV_DISPINFO member
item.pszText in the LVN_GETDISPINFO handler (and is valid). The destination
address is a pointer to the memory area where resources have been loaded: its
a valid address, but it's read-only, hence the crash. I don't know where this
value comes from, presumably somewhere inside comclt32.dll's listview control
implementation.
The comctl32.dll version that is loaded with the app has version
6.00.2900.2180.
Stephen C. Steel
> I'm seeing a crash that looks like it might be an internal problem with the
> listview control in the new theme-aware comctl32.dll version 6. ...
I've managed to reproduce this failure in a trivial test application: use
the VS .NET 2003 solution wizard and create a minimal MFC application
(statically linked MFC library, no doc/view support), but include an
application manifest (for new common controls library. Now add a dialog with
a report-mode owner-data list control. If the project is compiled for
multibyte support, it runs fine. If compiled for Unicode, I get a crash
identical to the one in the much larger application (_StringCopyWorkerW@12()
tries to copy the string I supplied for the first item into the read-only
memory section contraining resource-templates).
The dialog is invoked with a simple menu item:
void Cvlc_testApp::OnViewListdialog()
{
CDlgVlcTest dlg;
dlg.DoModal ();
}
and the diaglog code is pretty straightforward:
class CDlgVlcTest : public CDialog
{
DECLARE_DYNAMIC(CDlgVlcTest)
public:
CDlgVlcTest(CWnd* pParent = NULL); // standard constructor
virtual ~CDlgVlcTest();
// Dialog Data
enum { IDD = IDD_DIALOG1 };
protected:
virtual void DoDataExchange(CDataExchange* pDX); // DDX/DDV support
DECLARE_MESSAGE_MAP()
public:
CListCtrl m_list;
afx_msg void OnLvnGetdispinfoList1(NMHDR *pNMHDR, LRESULT *pResult);
};
void CDlgVlcTest::DoDataExchange(CDataExchange* pDX)
{
CDialog::DoDataExchange(pDX);
DDX_Control(pDX, IDC_LIST1, m_list);
if (!pDX->m_bSaveAndValidate)
{
// Initialize controls
// check resource settings for list control style bits
ASSERT (m_list.GetStyle () & LVS_REPORT);
ASSERT (m_list.GetStyle () & LVS_OWNERDATA);
m_list.InsertColumn (0, _T("First Col."), LVCFMT_LEFT);
m_list.InsertColumn (1, _T("Second Col."), LVCFMT_LEFT);
m_list.InsertColumn (2, _T("Third Col."), LVCFMT_LEFT);
m_list.SetColumnWidth (0, LVSCW_AUTOSIZE_USEHEADER);
m_list.SetColumnWidth (1, LVSCW_AUTOSIZE_USEHEADER);
m_list.SetColumnWidth (2, LVSCW_AUTOSIZE_USEHEADER);
m_list.SetItemCount (num_items);
}
}
BEGIN_MESSAGE_MAP(CDlgVlcTest, CDialog)
ON_NOTIFY(LVN_GETDISPINFO, IDC_LIST1, OnLvnGetdispinfoList1)
END_MESSAGE_MAP()
// CDlgVlcTest message handlers
void CDlgVlcTest::OnLvnGetdispinfoList1(NMHDR *pNMHDR, LRESULT *pResult)
{
NMLVDISPINFO *pDispInfo = reinterpret_cast<NMLVDISPINFO*>(pNMHDR);
ASSERT (pNMHDR->code == LVN_GETDISPINFO);
ASSERT (pNMHDR->idFrom == IDC_LIST1);
pDispInfo->item.mask = LVIF_TEXT;
ASSERT (pDispInfo->item.iItem < num_items);
if (pDispInfo->item.iSubItem == 0)
{
pDispInfo->item.pszText = const_cast <TCHAR *>(_T("Testing 1"));
}
else if (pDispInfo->item.iSubItem == 1)
{
pDispInfo->item.pszText = const_cast <TCHAR *>(_T("Testing 2"));
}
else if (pDispInfo->item.iSubItem == 2)
{
pDispInfo->item.pszText = const_cast <TCHAR *>(_T("Testing 3"));
}
else
{
ASSERT (0);
pDispInfo->item.pszText = const_cast <TCHAR *>(_T("What?"));
}
*pResult = 0;
}
David Lowndes
This code is wrong:
>void CDlgVlcTest::OnLvnGetdispinfoList1(NMHDR *pNMHDR, LRESULT *pResult)
>{
>...
> pDispInfo->item.pszText = const_cast <TCHAR *>(_T("Testing 1"));
You should copy your text to the buffer provided, like this:
void CDlgVlcTest::OnLvnGetdispinfoList1(NMHDR *pNMHDR, LRESULT
*pResult)
{
NMLVDISPINFO *pDispInfo =
reinterpret_cast<NMLVDISPINFO*>(pNMHDR);
ASSERT (pNMHDR->code == LVN_GETDISPINFO);
ASSERT (pNMHDR->idFrom == IDC_LIST1);
if ( ( pDispInfo->item.mask & LVIF_TEXT ) &&
(pDispInfo->item.pszText != NULL) )
{
static const LPCTSTR pT[3] =
{
_T("Testing 1"),
_T("Testing 2"),
_T("Testing 3")
};
if ( pDispInfo->item.iSubItem < _countof(pT) )
{
_tcscpy_s( pDispInfo->item.pszText,
pDispInfo->item.cchTextMax, pT[ pDispInfo->item.iSubItem ] );
}
}
*pResult = 0;
}
Stephen C. Steel
"David Lowndes" wrote:
I disagree, the code is not incorrect, one can either copy the string to the
buffer pointed to by pszText, or set pszText to a buffer maintained by your
application.
The Visual Studio .NET documentation for the NMLVDISPINFO structure has the
following statement under remarks:
<quote>
If the LVITEM structure is receiving item text, the pszText and cchTextMax
members specify the address and size of a buffer. You can either copy text to
the buffer or assign the address of a string to the pszText member. In the
latter case, you must not change or delete the string until the corresponding
item text is deleted or two additional LVN_GETDISPINFO messages have been
sent.
</quote>
Furthermore, this code has worked fine with many previous versions of
comctl32.dll, and ONLY fails for comtcl32.dll version 6 in Unicode apps.
David Lowndes
>buffer pointed to by pszText, or set pszText to a buffer maintained by your
>application.
>
>The Visual Studio .NET documentation for the NMLVDISPINFO structure has the
>following statement under remarks:
Yeah, you're right, sorry about that - I just thought it looked wrong
from memory and didn't read that section properly.
I can feel another subtle "by design" thing with the V6 common
controls here!
Since I could reproduce your problem with your code, all I can suggest
is to report it to MS (phone). I think it's either a lack of
documentation bug, or a bug in the V6 controls, and either way you
shouldn't be charged to report it. If you're an MSDN subscriber,
you've probably got some support requests available anyway.
Let us know how you go on.
Dave
David Liebtag
pszText to point at static storage that would remain unchanged after the
window procedure returned.
David Liebtag
Stephen C. Steel
But that's what I'm doing! In my trivial test app, pszText is pointing to a
statically allocated constant string: it exists when the app is loaded before
the first instruction of the is executed, and is there until the app has
exited. You can't get much more "unchanged" than that.
In the original application, the text came from a dynamically allocated
CString, but the CString existed before the offending dialog was created, and
until long after the dialog was destroyed. So it wasn't a lifetime issue on
my end.
To give credit where it is due, the NMLVDISPINFO documentation did actually
discuss the lifetime requirements of it's arguements. This makes it quite
noteworthy, since lifetime/ownership requirements of arguments is an aspect
that the overwhelming majority of Microsoft API documentation just skips
(with entirely predictable results).
Stephen C. Steel
> I can feel another subtle "by design" thing with the V6 common
> controls here!
What kind of "design" would say that this long standing behaviour should be
OK with MCBS apps, but cause a crash with Unicode? That feels more
like"rationalization of a bug we don't want to bother fixing" than "by
design". After all, would they issue a hot-fix to cause the Unicode version
to crash by design if it had turned out that it was working like earlier
comclt32.dll versions by accident?
> Since I could reproduce your problem with your code, all I can suggest
> is to report it to MS (phone). I think it's either a lack of
> documentation bug, or a bug in the V6 controls, and either way you
> shouldn't be charged to report it. If you're an MSDN subscriber,
> you've probably got some support requests available anyway.
I did report the problem to Microsoft about a week ago via the
connect.microsoft.com website (the submission included a ZIP archive of the
VS project illustrating the crash). So far, I haven't heard anything.
Stephen C. Steel
"David Lowndes" wrote:
> This code is wrong:
And he was correct, my code IS wrong.
> You should copy your text to the buffer provided, like this:
But this was not the reason (see my earlier reply).
The mistake was the line
pDispInfo->item.mask = LVIF_TEXT;
Instead, I should have been testing if the LVIF_TEXT bit had been set by
comctl32.dll and doing nothing (apart from setting *pResult=0) if it wasn't.
It appears the new comctl32.dll library sends a notifications without
LVIF_TEXT set (e.g. just LVIF_INDENT), and didn't respond well when I set
pszText in response.
Maybe the older comctl32.dll versions only sent a single notification with
multiple bits set (with LVIF_TEXT among them) so it was always OK to set
pszText unconditionally. This would explain why this code worked up until
now.
You can still have problems if you copy into the buffer but don't test mask
to see if the LVIF_TEXT bit is set. If LVIF_TEXT isn't set, the cchTextMax
member seems to have a meaningless (and very large) value. This could cause
problems if you tried to fill the buffer!
David Lowndes
>connect.microsoft.com website (the submission included a ZIP archive of the
>VS project illustrating the crash). So far, I haven't heard anything.
Can you post a link to your bug report item here and I'll validate it.
Dave
David Lowndes
>
>pDispInfo->item.mask = LVIF_TEXT;
Ah, I had spotted that and fixed it in the code I posted, I just
forgot to point it out as I assumed the root problem was (what I
thought) elsewhere.
Good to know anyhow.
Dave
Stephen C. Steel
> Can you post a link to your bug report item here and I'll validate it.
If I could figure out what link to post, I'd happily do so. I don't recall
being given a URL or link to refer to (and I didn't receive a subsequent
e-mail either). I submitted it via this URL
on 2006-07-18 (from the passport account associated with st...@kvs.com).
Since I found I can avoid the crash by checking that LVIF_TEXT is set and
only setting pszText when it is, the matter is less urgent for me (see posts
below).
I discovered that both comctl32.dll versions shipped with XP-sp2 send a
LVN_GETDISPINFO notification with just the LVIF_INDENT bit set. However, the
older version doesn't crash if I set pszText, only the newer version does.
This suggests there is some fragile code in comctl32.dll version 6: it should
just ignore itperfectly valid data that it doesn't care about rather than
crashing.
Another nasty I noticed: when comctl32.dll v6 sends the LVN_GETDISPINFO
notification with mask = LVIF_INDENT, the cchTextMax member appears to be
unitialized data (typically a large value). If the application code tries to
copy a long string limited by cchTextMax characters to pszText, things get
messy. It would be much more robust if the pszText and cchTextData members
were either both valid in all notifications, or if they were cleared to NULL
and 0 respectively when the LVN_GETDISPINFO is sent without LVIF_TEXT set.
David Lowndes
>
>If I could figure out what link to post, I'd happily do so.
If you log back onto the connect site, your bug report should be
listed in your feedback. I usually just right click on the bug report
link there and use "copy shortcut".
Anyhow, I guess it doesn't matter now you've sussed it, though it
might be good to add a follow up comment to the report to clarify
things to someone at MS who'll look at the report.
Cheers
Dave
Stephen C. Steel
"David Lowndes" wrote:
> >> Can you post a link to your bug report item here and I'll validate it.
> >
> >If I could figure out what link to post, I'd happily do so.
>
> If you log back onto the connect site, your bug report should be
> listed in your feedback. I usually just right click on the bug report
> link there and use "copy shortcut".
This time I managed to find it:
http://connect.microsoft.com/VisualStudio/feedback/ViewFeedback.aspx?FeedbackID=166088