View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0000444 | Far Manager | History | public | 2007-12-15 10:07 | 2007-12-24 06:36 |
Reporter | vsergiy | Assigned To | vskirdin | ||
Priority | normal | Severity | major | Reproducibility | always |
Status | closed | Resolution | fixed | ||
Fixed in Version | 1.80 alpha 1 | ||||
Summary | 0000444: Bugs and memory leaks in class History | ||||
Description | 1. Ошибка в алгоритме добавления в историю с удалением дубликатов – memory leak 2. Ошибка при добавлении в историю с отключенными дубликатами – memory leak 3. Ошибка при сохранении "Types" – сохранялись не в юникоде. Это вызывало ошибку при чтении истории и ошибки при дальнейшей работе с историей. Патч в комплекте. | ||||
Tags | No tags attached. | ||||
Build | 395 | ||||
2007-12-15 10:07
|
history.cpp.patch (3,559 bytes)
Index: history.cpp =================================================================== --- history.cpp (revision 1402) +++ history.cpp (working copy) @@ -216,10 +216,6 @@ RemoveTrailingSpaces(AddRecord.Name); AddRecord.Type=Type; - int OldLastPtr=LastPtr-1; - if (OldLastPtr < 0) - OldLastPtr=HistoryCount-1; - if (RemoveDups) // ����� ��������? { HistoryRecord *PtrLastStr; @@ -246,12 +242,16 @@ if (Equal) { + int OldLastPtr=LastPtr-1; + if (OldLastPtr < 0) + OldLastPtr=HistoryCount-1; + int Length=OldLastPtr-I; if (Length<0) Length+=HistoryCount; - for (J=0; J <= Length; J++) + for (J=0; J < Length; J++) { int Dest=(I+J) % HistoryCount; int Src=(I+J+1) % HistoryCount; @@ -265,6 +265,10 @@ memset(LastStr+Src,0,sizeof(HistoryRecord)); } + if (Length==0) + if(LastStr[OldLastPtr].Name) + xf_free(LastStr[OldLastPtr].Name); + memcpy(LastStr+OldLastPtr, &AddRecord, sizeof(HistoryRecord)); CurLastPtr0=LastPtr0=CurLastPtr=LastPtr; @@ -274,18 +278,13 @@ } } - int Pos=(LastPtr-1) % HistoryCount; - - if(LastStr[Pos].Name && LastStr[LastPtr].Name && - (StrCmp(AddRecord.Name,LastStr[Pos].Name) != 0 || - StrCmp(AddRecord.Title,LastStr[Pos].Title) != 0 || - !EqualType(AddRecord.Type,LastStr[Pos].Type))) + if(LastStr[LastPtr].Name) xf_free(LastStr[LastPtr].Name); memcpy(LastStr+LastPtr,&AddRecord,sizeof(HistoryRecord)); if (++LastPtr==HistoryCount) - LastPtr=0; + LastPtr=0; CurLastPtr0=LastPtr0=CurLastPtr=LastPtr; } @@ -296,14 +295,14 @@ BOOL History::SaveHistory() { if(!LastStr) - return FALSE; //?? + return FALSE; if (!*EnableSave) return TRUE; wchar_t *BufferLines=NULL,*BufferTitles=NULL,*PtrBuffer; - unsigned char *TypesBuffer; - TypesBuffer=(unsigned char *)alloca(HistoryCount+1); + wchar_t *TypesBuffer; + TypesBuffer=(wchar_t *)alloca((HistoryCount+1)*sizeof(wchar_t)); if(!TypesBuffer) return FALSE; @@ -352,11 +351,10 @@ if (SaveType) { - memset(TypesBuffer,0,HistoryCount+1); + memset(TypesBuffer,0,(HistoryCount+1)*sizeof(wchar_t)); for (I=0; I < HistoryCount; I++) if(LastStr[I].Name) - TypesBuffer[SizeTypes++]=LastStr[I].Type+'0'; - TypesBuffer[SizeTypes++]=0; + TypesBuffer[SizeTypes++]=LastStr[I].Type+L'0'; } } @@ -378,7 +376,7 @@ if (SaveType) { if(TypesBuffer && *TypesBuffer) - RegSetValueExW(hKey,L"Types",0,REG_SZ,TypesBuffer,SizeTypes); + RegSetValueExW(hKey,L"Types",0,REG_SZ,(unsigned char *)TypesBuffer,(SizeTypes+1)*sizeof(wchar_t)); else RegDeleteValueW(hKey,L"Types"); } @@ -497,10 +495,10 @@ if (NeedReadType) { unsigned char *TypesBuffer; - TypesBuffer=(unsigned char *)alloca(HistoryCount+2); + TypesBuffer=(unsigned char *)alloca((HistoryCount+1)+sizeof(wchar_t)); if(TypesBuffer) - memset(TypesBuffer,0,HistoryCount+1); - Size=HistoryCount+1; + memset(TypesBuffer,0,(HistoryCount+1)*sizeof(wchar_t)); + Size=(HistoryCount+1)*sizeof(wchar_t); if(TypesBuffer && RegQueryValueExW(hKey,L"Types",0,&Type,(unsigned char *)TypesBuffer,&Size)==ERROR_SUCCESS) { StrPos=0; |
|
1. А... разве вот здесь @@ -265,6 +265,10 @@ memset(LastStr+Src,0,sizeof(HistoryRecord)); } + if (Length==0) + if(LastStr[OldLastPtr].Name) + xf_free(LastStr[OldLastPtr].Name); + memcpy(LastStr+OldLastPtr, &AddRecord, sizeof(HistoryRecord)); после xf_free обнулять Name не надо? LastStr[OldLastPtr].Name=NULL; 2. Поясни, pls, вот этот кусок: - int Pos=(LastPtr-1) % HistoryCount; - - if(LastStr[Pos].Name && LastStr[LastPtr].Name && - (StrCmp(AddRecord.Name,LastStr[Pos].Name) != 0 || - StrCmp(AddRecord.Title,LastStr[Pos].Title) != 0 || - !EqualType(AddRecord.Type,LastStr[Pos].Type))) + if(LastStr[LastPtr].Name) xf_free(LastStr[LastPtr].Name); Почему удалил? |
|
на счет п.1 пред.поста сморозил, sorry :-) |
|
1. Зачем рассчитывать значение “Pos” (int Pos=(LastPtr-1) % HistoryCount), а потом еще и проверять имя элемента с этим индексом ( if(LastStr[Pos].Name && ...) ) если запись производиться в элемент с индексом “LastPtr”? 2. Почему освобождается память только в том случае если записи различны? Даже если записи одинаковы, нужно все равно освободить память выделенную для имени прежде чем копировать новую запись. Все что нужно сделать это удалить запись которая находиться в позиции “LastPtr” (если она там есть) и на ее место скопировать новую запись после чего сместить указатель “LastPtr”. |
|
см. после 395 |
Date Modified | Username | Field | Change |
---|---|---|---|
2007-12-15 10:07 | vsergiy | New Issue | |
2007-12-15 10:07 | vsergiy | File Added: history.cpp.patch | |
2007-12-21 07:18 | vskirdin | Note Added: 0001058 | |
2007-12-21 07:26 | vskirdin | Note Added: 0001060 | |
2007-12-21 17:00 | vsergiy | Note Added: 0001062 | |
2007-12-23 05:03 | vskirdin | Status | new => assigned |
2007-12-23 05:03 | vskirdin | Assigned To | => vskirdin |
2007-12-23 05:05 | vskirdin | Note Added: 0001066 | |
2007-12-23 05:05 | vskirdin | Status | assigned => feedback |
2007-12-24 01:52 | vsergiy | Status | feedback => resolved |
2007-12-24 01:52 | vsergiy | Resolution | open => fixed |
2007-12-24 06:36 | vskirdin | Build | => 395 |
2007-12-24 06:36 | vskirdin | Status | resolved => closed |
2007-12-24 06:36 | vskirdin | Fixed in Version | => 1.80 alpha 1 |