View Issue Details

IDProjectCategoryView StatusLast Update
0000447Far ManagerEditorpublic2008-01-10 00:12
Reporteralexmitin Assigned Tovskirdin  
PrioritynormalSeveritymajorReproducibilityalways
Status closedResolutionfixed 
Fixed in Version1.80 alpha 1 
Summary0000447: Buffer overrun in Edit:ProcessKey()
DescriptionSteps to reproduce
======================
1) Start FAR under debugger.
2) Put a breakpoint on the following line:

        wchar_t *ShortStr=new wchar_t[StrSize+1];

See the code below for the full context.

3) Press Ctrl+A on the file or folder
4) Select "File modification date" control.
5) Press Right arrow button.
6) Step through the code and watch the value of "ShortStr" variable.


File: edit.cpp
Line: 1024-1025
Context:

int Edit::ProcessKey(int Key)
{
...
    case KEY_RIGHT: case KEY_NUMPAD6:
    case KEY_CTRLD:
    {
      PrevCurPos=CurPos;

      if (Mask && *Mask)
      {
        wchar_t *ShortStr=new wchar_t[StrSize+1];
        if (ShortStr==NULL)
          return FALSE;
        wcsncpy(ShortStr,Str,StrSize); <==[bug]==
        int Len=StrLength(RemoveTrailingSpaces(ShortStr));
        delete[] ShortStr;
        if (Len>CurPos)
          CurPos++;
      }
...
}

Analysis
=========
The strncpy function copies the initial count characters of strSource to strDest and returns strDest. If count is less than or equal to the length of strSource, a null character is not appended automatically to the copied string. If count is greater than the length of strSource, the destination string is padded with null characters up to length count. The behavior of strncpy is undefined if the source and destination strings overlap.

See http://msdn2.microsoft.com/en-us/library/xdsywd25(VS.80).aspx
-----

In my test run the value of the variables were the following:

StrSize=10
Str=12/18/2007

In this particular case the function "wcsncpy" does not null terminate "ShortStr". The next line calls RemoveTrailingSpaces, which in turns calls StrLength, which calls lstrlenW.

Because "ShortStr" is not null terminated lstrlenW reads after the allocated buffer. In some cases it can cause access violation error.

There are several other places like this in the Edit::ProcessKey function. They all should be fixed.


Fix
===========
wchar_t *ShortStr=new wchar_t[StrSize+1];
if (ShortStr==NULL)
   return FALSE;
lstrcpynW(ShortStr,Str,StrSize+1); <==[fix]==
int Len=StrLength(RemoveTrailingSpaces(ShortStr));
delete[] ShortStr;
Additional InformationFAR 1.80.0.394
TagsNo tags attached.
Build407

Activities

samlyukov

2007-12-18 09:55

reporter   bugnote:0001051

есть же в strncpy.c xwcsncpy()

vskirdin

2007-12-21 05:41

administrator   bugnote:0001055

после 395

samlyukov

2007-12-24 01:24

reporter   bugnote:0001071

Валь, не то.
См. аттач.

2007-12-24 01:25

 

work.diff (8,437 bytes)   
Index: panel.cpp
===================================================================
--- panel.cpp	(revision 1406)
+++ panel.cpp	(working copy)
@@ -969,8 +969,7 @@
 		{
 			wchar_t RootDir[10]; //BUGBUG
 
-			wcsncpy(RootDir,strCurDir,3);
-			RootDir[3]=0;
+			xwcsncpy(RootDir,strCurDir,3);
 
 			if ( FAR_GetDriveType(RootDir) == DRIVE_NO_ROOT_DIR )
 				ChDisk.ClearDone();
Index: edit.cpp
===================================================================
--- edit.cpp	(revision 1406)
+++ edit.cpp	(working copy)
@@ -765,7 +765,7 @@
         if (ShortStr==NULL)
           return FALSE;
 
-        wcsncpy(ShortStr,Str,StrSize);
+        xwcsncpy(ShortStr,Str,StrSize);
         Len=StrLength(RemoveTrailingSpaces(ShortStr));
         delete[] ShortStr;
       }
@@ -988,7 +988,7 @@
         wchar_t *ShortStr=new wchar_t[StrSize+1];
         if (ShortStr==NULL)
           return FALSE;
-        wcsncpy(ShortStr,Str,StrSize);
+        xwcsncpy(ShortStr,Str,StrSize);
         CurPos=StrLength(RemoveTrailingSpaces(ShortStr));
         delete[] ShortStr;
       }
@@ -1021,7 +1021,7 @@
         wchar_t *ShortStr=new wchar_t[StrSize+1];
         if (ShortStr==NULL)
           return FALSE;
-        lstrcpynW(ShortStr,Str,StrSize+1);
+        xwcsncpy(ShortStr,Str,StrSize);
         int Len=StrLength(RemoveTrailingSpaces(ShortStr));
         delete[] ShortStr;
         if (Len>CurPos)
@@ -1117,7 +1117,7 @@
         wchar_t *ShortStr=new wchar_t[StrSize+1];
         if (ShortStr==NULL)
           return FALSE;
-        wcsncpy(ShortStr,Str,StrSize);
+        xwcsncpy(ShortStr,Str,StrSize);
         Len=StrLength(RemoveTrailingSpaces(ShortStr));
         delete[] ShortStr;
         if (Len>CurPos)
@@ -1162,7 +1162,7 @@
             wchar_t *ShortStr=new wchar_t[StrSize+1];
             if (ShortStr==NULL)
               return FALSE;
-            wcsncpy(ShortStr,Str,StrSize);
+            xwcsncpy(ShortStr,Str,StrSize);
             RemoveTrailingSpaces(ShortStr);
             CopyToClipboard(ShortStr);
             delete[] ShortStr;
@@ -1443,7 +1443,6 @@
 void Edit::GetString(wchar_t *Str,int MaxSize)
 {
     xwcsncpy(Str, Edit::Str,MaxSize-1);
-    Str[MaxSize-1]=0;
 }
 
 void Edit::GetString(string &strStr)
@@ -1612,8 +1611,7 @@
   else
     CopyLength=Min(MaxSize-1,SelEnd-SelStart);
 
-  wcsncpy(Str,Edit::Str+SelStart,CopyLength);
-  Str[CopyLength]=0;
+  xwcsncpy(Str,Edit::Str+SelStart,CopyLength);
 
   return(TRUE);
 }
@@ -1633,8 +1631,7 @@
 
   wchar_t *lpwszStr = strStr.GetBuffer (CopyLength+1);
 
-  wcsncpy(lpwszStr,Edit::Str+SelStart,CopyLength);
-  lpwszStr[CopyLength]=0;
+  xwcsncpy(lpwszStr,Edit::Str+SelStart,CopyLength);
 
   strStr.ReleaseBuffer ();
 
@@ -1976,7 +1973,7 @@
     wchar_t *ShortStr=new wchar_t[StrSize+1];
     if (ShortStr==NULL)
       return;
-    wcsncpy(ShortStr,Str,StrSize);
+    xwcsncpy(ShortStr,Str,StrSize);
     Pos=StrLength(RemoveTrailingSpaces(ShortStr));
     delete[] ShortStr;
     if (NewPos>Pos)
Index: mix.cpp
===================================================================
--- mix.cpp	(revision 1406)
+++ mix.cpp	(working copy)
@@ -226,7 +226,7 @@
     if ( Result &&
          IsAlpha (*lpwszBuffer) &&
          lpwszBuffer[1] == L':' &&
-         (lpwszBuffer[2] == 0 || lpwszBuffer[2] == '\\')
+         (lpwszBuffer[2] == 0 || lpwszBuffer[2] == L'\\')
          )
          *lpwszBuffer = Upper (*lpwszBuffer);
 
@@ -443,7 +443,7 @@
 
       int nResult = ConvertWildcards(param1, strResult, flags & 0xFFFF);
 
-      xwcsncpy(param2, strResult, size);
+      xwcsncpy(param2, strResult, size); //?? � ����� �� size-1
 
       return nResult;
   }
@@ -1066,7 +1066,7 @@
 
     if ( FarMkTempEx(strDest, Prefix, TRUE) )
     {
-        xwcsncpy (Dest, strDest, size);
+        xwcsncpy (Dest, strDest, size);  //?? � ����� �� size-1
         return Dest;
     }
 
@@ -1121,7 +1121,7 @@
 
   if(DriveType == DRIVE_UNKNOWN)
   {
-    LocalName[2]='\\';
+    LocalName[2]=L'\\';
     DriveType = FAR_GetDriveType(LocalName);
     LocalName[2]=0;
   }
@@ -1768,8 +1768,7 @@
     */
     wchar_t LocalName[3];
 
-    wcsncpy (LocalName, CurDir, 2);
-    LocalName[2] = 0;
+    xwcsncpy (LocalName, CurDir, 2);
 
     apiWNetGetConnection (LocalName, strNetDir);
   }
Index: fnparce.cpp
===================================================================
--- fnparce.cpp	(revision 1406)
+++ fnparce.cpp	(working copy)
@@ -836,7 +836,7 @@
 
     if(Modifers && *Modifers)
     {
-      if(wcschr(Modifers,'F') && PointToName((const wchar_t*)strFileName) == (const wchar_t*)strFileName) // 'F' - ���������� ����� ���; //BUGBUG
+      if(wcschr(Modifers,L'F') && PointToName((const wchar_t*)strFileName) == (const wchar_t*)strFileName) // 'F' - ���������� ����� ���; //BUGBUG
       {
         string strTempFileName;
         strTempFileName.Format (L"%s%s%s", (const wchar_t*)strCurDir,(strCurDir.At(StrLength(strCurDir)-1) != L'\\'?L"\\":L""), (const wchar_t*)strFileName); //BUGBUG
Index: syntax.cpp
===================================================================
--- syntax.cpp	(revision 1406)
+++ syntax.cpp	(working copy)
@@ -300,7 +300,7 @@
   static wchar_t buffer[128];
   wchar_t *p = buffer;
   int ch;
-  while ( ( ( ch = getChar() ) != EOFCH ) && (iswxdigit(ch) || ch == 'x') && ( (p-buffer) < 32 ))
+  while ( ( ( ch = getChar() ) != EOFCH ) && (iswxdigit(ch) || ch == L'x') && ( (p-buffer) < 32 ))
     *p++ = (char)ch;
   *p = 0;
   putBack(ch);
@@ -725,8 +725,7 @@
 
    wchar_t *CurKeyText = strCurKeyText.GetBuffer (Length+1);
 
-   wcsncpy(CurKeyText,CurBufPtr,Length);
-   CurKeyText[Length] = 0;
+   xwcsncpy(CurKeyText,CurBufPtr,Length);
 
    strCurKeyText.ReleaseBuffer ();
 
Index: macro.cpp
===================================================================
--- macro.cpp	(revision 1406)
+++ macro.cpp	(working copy)
@@ -1276,7 +1276,7 @@
       p = es;
 
     if ( (nFlags & FLAG_DISK) == FLAG_DISK )
-      wcsncpy (lpDest, s, p-s);
+      wcsncpy (lpDest, s, p-s);               //??! xwcsncpy()
   }
   else
   {
Index: UNICODE
===================================================================
--- UNICODE	(revision 1406)
+++ UNICODE	(working copy)
@@ -14,7 +14,7 @@
    strcat          wcscat     
    strncat         wcsncat
    strcpy          wcscpy
-   strncpy         wcsncpy
+   xstrncpy        xwcsncpy       see file "strncpy.c"
    strcmp          StrCmp
    _stricmp        StrCmpI
    strncmp         StrCmpN
Index: clipboard.cpp
===================================================================
--- clipboard.cpp	(revision 1406)
+++ clipboard.cpp	(working copy)
@@ -341,7 +341,6 @@
       wmemset(ClipText,0,BufferSize+2);
 
       xwcsncpy(ClipText,ClipAddr,BufferSize);
-      ClipText[BufferSize]=0;
 
 /*      if (Unicode)
         if(AnsiMode)
Index: strmix.cpp
===================================================================
--- strmix.cpp	(revision 1406)
+++ strmix.cpp	(working copy)
@@ -257,8 +257,7 @@
 
 	PartBeforeName = (wchar_t*)xf_malloc ((BeforeNameLength+1)*sizeof (wchar_t));
 
-	wcsncpy(PartBeforeName, Src, BeforeNameLength);
-	PartBeforeName[BeforeNameLength]=0;
+	xwcsncpy(PartBeforeName, Src, BeforeNameLength);
 
 	const wchar_t *SrcNameDot=wcsrchr(SrcNamePtr, L'.');
 
@@ -288,7 +287,7 @@
 					*(DestNamePtr++)=*(SrcNamePtr++);
 				}
 				break;
-			case '.':
+			case L'.':
 				CurWildPtr++;
 				*(DestNamePtr++)=L'.';
 				if (wcspbrk(CurWildPtr,L"*?")!=NULL)
Index: editor.cpp
===================================================================
--- editor.cpp	(revision 1406)
+++ editor.cpp	(working copy)
@@ -3538,7 +3538,6 @@
             const wchar_t *Str=CurPtr->GetStringAddrW()+CurPtr->GetCurPos();
             wchar_t *TmpStr=new wchar_t[SearchLength+1];
             xwcsncpy(TmpStr,Str,SearchLength);
-            TmpStr[SearchLength]=0;
 
             /*
             if (UseDecodeTable)
@@ -4279,7 +4278,6 @@
   UndoData[UndoDataPos].StrPos=StrPos;
   UndoData[UndoDataPos].StrNum=StrNum;
   xwcsncpy(UndoData[UndoDataPos].EOL,Eol?Eol:L"",countof(UndoData[UndoDataPos].EOL)-1);
-  UndoData[UndoDataPos].EOL[countof(UndoData[UndoDataPos].EOL)-1]=0;
 
   if (Str!=NULL)
   {
work.diff (8,437 bytes)   

samlyukov

2008-01-09 10:18

reporter   bugnote:0001108

Так что с аттачем будем делать?

vskirdin

2008-01-09 10:55

administrator   bugnote:0001111

после 407

Issue History

Date Modified Username Field Change
2007-12-18 09:16 alexmitin New Issue
2007-12-18 09:16 alexmitin Status new => assigned
2007-12-18 09:16 alexmitin Assigned To => vskirdin
2007-12-18 09:55 samlyukov Note Added: 0001051
2007-12-21 05:41 vskirdin Note Added: 0001055
2007-12-21 05:41 vskirdin Status assigned => feedback
2007-12-24 01:24 samlyukov Note Added: 0001071
2007-12-24 01:25 samlyukov File Added: work.diff
2008-01-09 10:18 samlyukov Note Added: 0001108
2008-01-09 10:55 vskirdin Note Added: 0001111
2008-01-09 11:19 samlyukov Status feedback => resolved
2008-01-09 11:19 samlyukov Resolution open => fixed
2008-01-10 00:12 vskirdin Build => 407
2008-01-10 00:12 vskirdin Status resolved => closed
2008-01-10 00:12 vskirdin Fixed in Version => 1.80 alpha 1