Bugzilla – Bug 4330
ArrayInsert...Before/After Overflow?
Last modified: 2013-06-25 08:04:59 PDT
I believe I've found a bug in the dynamic array natives that use the ShiftUpFrom function in datastructs.h. (ArrayInsertStringBefore etc...) Basically my game was freezing when attempting to insert an item before another when the dynamic array was at its reserved size I believe the problem is the last line of code below causing an overflow. Possibly because the tempbuffersize doesn't take into account count being incremented by Push()?? // First make a new item. this->Push(); // If we got an InsertAfter(lastitem), then which will equal this->count - 1 // all we needed to do was Push() if (which == this->count || which == this->count - 1) { return 1; } // Allocate a temporary buffer to store data in size_t tempbuffsize=(sizeof(cell) * cellcount) * (this->count - which); cell* temp=(cell*)malloc(tempbuffsize); // Copy old data to temp buffer memcpy(temp, GetCellPointer(which), tempbuffsize); // Now copy temp buffer to adjusted location memcpy(GetCellPointer(which+1), temp, tempbuffsize);
Created attachment 2009 [details] Reproduces bug
Would an acceptable fix be changing: size_t tempbuffsize=(sizeof(cell) * cellcount) * (this->count - which); to size_t tempbuffsize=(sizeof(cell) * cellcount) * (this->count - 1 - which); ?
Sawce - this is your code, any thoughts on comment #2?
Bump. I've noticed this isn't fixed as of the latest amxmodx, I've been using the fix mentioned since posting and it has worked fine.
Created attachment 3383 [details] Proposed patch I've investigated the issue and can confirm the bug. The actual problem is when the reserved size is x and you push up to x - 1 elements, and when ArrayInsert* are used at x - 2, such natives push before a blank element before insertion (of the real value). tempbuffsize is then calculated using this->count - which to know the size of the block of elements to shift. The bug is since a new element has been added before, this->count has been incremented, resulting the blank element be used and making wrong the size of blocks of elements to shift. As consequence, when the block is inserted at x - 1, data copied overflows the reserved size. A solution would be to not count the push done before shifting to properly calculate the size of block of elements to shift.
Pushed changeset: http://hg.alliedmods.net/amxmodx-central/rev/0b8cc5bde8b6 Changelog: Fix an overflow issue with ArrayInsert[Before|After] (bug 4330, r=joropito)