Bug 4330 - ArrayInsert...Before/After Overflow?
ArrayInsert...Before/After Overflow?
Status: RESOLVED FIXED
Product: AMX Mod X
Classification: Unclassified
Component: Core
trunk
PC All
: P3 normal
Assigned To: amxmodx-bugs@alliedmods.net
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-03-28 22:45 PDT by watch
Modified: 2013-06-25 08:04 PDT (History)
4 users (show)

See Also:


Attachments
Reproduces bug (240 bytes, text/plain)
2010-03-29 09:37 PDT, watch
no flags Details
Proposed patch (453 bytes, text/plain)
2013-06-21 17:37 PDT, Vincent Herbet [:Arkshine]
hv.contact: review+
joropito: feedback+
Details

Description watch 2010-03-28 22:45: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);
Comment 1 watch 2010-03-29 09:37:37 PDT
Created attachment 2009 [details]
Reproduces bug
Comment 2 watch 2010-03-29 09:41:04 PDT
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);

?
Comment 3 David Anderson [:dvander] 2010-04-27 02:37:21 PDT
Sawce - this is your code, any thoughts on comment #2?
Comment 4 watch 2012-01-23 13:53:25 PST
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.
Comment 5 Vincent Herbet [:Arkshine] 2013-06-21 17:37:33 PDT
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.
Comment 6 Vincent Herbet [:Arkshine] 2013-06-25 07:59:42 PDT
Pushed changeset: http://hg.alliedmods.net/amxmodx-central/rev/0b8cc5bde8b6
Changelog:
	Fix an overflow issue with ArrayInsert[Before|After] (bug 4330, r=joropito)

Note You need to log in before you can comment on or make changes to this bug.