1

In order to fix a buffer overflow Coverity issue, I have used strncpy() to copy a list item. The list item needs to be drag and dropped from one row to another. So the string that needs to be copied contains '\n', '\t' and ' ' characters.

I have used the below code. 

for (int nColumn = 1; nColumn < nColumns; nColumn++)
{
    strncpy(lvItem.pszText, (LPCTSTR)(GetItemText(nDragIndex, nColumn)), sizeof(lvItem.pszText)-1);
    lvItem.pszText[sizeof(lvItem.pszText)] = '\0';
    lvItem.iSubItem = nColumn;
    SetItem(&lvItem);
}

The Coverity scan passed but the data in some of the columns gets truncated. I have heard of using strcpy_s method but is not available. Can anyone help me resolve issue?

Prasanth
  • 75
  • 8
  • Remove the line lvItem.pszText[sizeof(lvItem.pszText)] = '\0'; entirely. The string is already null-terminated –  Sep 03 '18 at 13:46
  • 1
    @peakpeak -- the **source** string is nul-terminated. Adding the `'\0'` is an attempt to work around using the wrong tool. `strncpy` does not add a nul terminator if it hits the end of the target range. It was never intended to be a "safe" alternative to `strcpy`. – Pete Becker Sep 03 '18 at 13:51
  • `(LPCTSTR)` is likely part of your problem. I mean when you have to cast string pointers you are likely casting from the wrong type. – drescherjm Sep 03 '18 at 13:57

1 Answers1

2

Your code is wrong and doesn't do what you expect. lvItem.pszText is a pointer and it has a fixed size of 4 respective 8 bytes depending on the kind of your project. So your sizeof operator causes the truncation.

Using LVITEM in this way, need a buffer that this defined by you!

If you use GetIemText, you can also user CListCtrl::SetItemText This function takes care about all limits.

Jabberwocky
  • 48,281
  • 17
  • 65
  • 115
xMRi
  • 14,982
  • 3
  • 26
  • 59