Managed struct this-pointer weirdness (AGS reference count bug?)

Started by Snarky, Fri 20/10/2017 19:19:36

Previous topic - Next topic

Snarky

I'm experiencing... weirdness, and sometimes crashes, in methods of managed structs that use the this-pointer, if I delete/overwrite all external handles to the object partway through the method.

The setup is something like this:

Code: ags
// Simplified struct
managed struct Job
{
  int value;
};

// A different script:

// I need to keep track of a bunch of jobs
Job* jobPool[100];

// This function manipulates the job pool
void Process(this Job*, int index)
{
  Display("This Job rating: %d", this.value); // I access the value

  // THE IMPORTANT BIT:
  // Then delete a particular entry in the jobPool
  jobPool[index] = null;
  // ... and create a new Job
  Job* job = new Job;
  job.value = 20;

  Display("This Job rating recheck: %d", this.value); // I access the value FOR THIS JOB again
}

// And I call that function:
void someOtherFunction()
{
  // Fill the job pool
  for(int i=0; i<100; i++)
  {
    jobPool[i] = new Job;
    jobPool[i].value = i*2;
  }

  // ... and later on
  jobPool[5].Process(5); // The SAME NUMBER
}


So I'm calling Job.Process() on jobPool[5], telling it to remove the entry in jobPool[5] from the array (in my real program, Job.Process() handles the indexes and always removes the job you call it on from the array, but let's keep it simple here). Since this is done with pointers, the object itself should not be affected (though we won't have any way to reference it after the function completes).

However, what actually happens is that it Displays:

This Job rating: 10 (the original value, correctly)
This Job rating recheck: 20 (the value of the new Job we made, job)

In other words, the this pointer is now pointing to the wrong Job!

If I remove the jobPool[index] = null; on line 19, this doesn't happen. I'm guessing that since the only external reference to this object is the entry in jobPool[5], setting that to null reduces its reference count to 0 and marks the object as deleted, even though we're still inside a function that has a local pointer to it, and that the new object is therefore slotted into the newly "free" memory, so that the this-pointer ends up pointing to it by "accident" (supporting this theory, if I add the line Job* j = jobPool[5]; on line 36 right before I call jobPool[5].Process(5); it also doesn't happen).

Under other circumstances, this can cause AGS to crash with the error "Pointer cast failure: the object being pointed to is not in the managed object pool", but I'm not able to recreate that crash in a simple sample program.

Crimson Wizard

I can confirm this, issue can be reproduced even with built-in AGS objects, which means it's a general bug:

Code: ags

DynamicSprite *globalSprite;

void TestFunc(this DynamicSprite*) {
  Display("Sprite w: %d, h: %d", this.Width, this.Height);
  globalSprite = null;
  DynamicSprite *localSprite = DynamicSprite.Create(20, 20);
  Display("Sprite w: %d, h: %d", this.Width, this.Height);
}

function on_key_press(eKeyCode keycode) 
{
  if (keycode == eKey0) {
    globalSprite = DynamicSprite.Create(10, 10);
    globalSprite.TestFunc();
  }
}


Upon pressing key '0', following is displayed:
Sprite w: 10, h: 10
Sprite w: 20, h: 20

Interestingly, following change to function prototype does not cause same issue:
Code: ags
void TestFunc(DynamicSprite* spr)


The reason might be that when "this" is passed to function it does not get reference count increased, nor count decreased when function is exited (but it should).
This is pretty dangerous, actually, because if you just remove sprite from array, using "this" afterwards will address... not sure what part of memory: either cleaned up buffer, or outside of managed pool (which in theory could be the case of the crash you mentioned).


EDIT: Same happens in AGS 3.2.1, looks like another ancient bug in AGS.

Snarky

Thanks for confirming, Crimson Wizard! That also suggests a simple workaround: at the top of the function body you can just add something like (in your example):

Code: ags
  DynamicSprite* this_ds = this;


Doing so does increase the reference count, and the bug is averted. (Of course, it would still be nice to have it fixed!)

SMF spam blocked by CleanTalk