ResFract.c use of ResCheckConcavity() may split tiles (underneath, is that ok?)

[dlmiles]

The context here is TiFree() was called from TiJoinY() which is called from ResSplitX() from the same file linked.

The call chain: ResCheckConcavity->resWalkdown->ResSplitX->TiTileY->TiFree freed resSrTile, then return back, then deref of TOP(resSrTile) which has just has TiFree() applied to it.

So under normal malloc/free style rules applied to TiAlloc()/TiFree() that is a use-after-free in the dereference of TOP(resSrTile) (in this scenario) but it looks like it could have equally been the BOTTOM(resTopTile) as ResCheckConcavity() is free to pick either side and perform a split/join operations.

Also to remind that after calling TiFree() the ti_client is invalidated (due to being added to free-Tile linked-list).
I am not sure (at this time) if any of the other fields are invalidated/changed in the original tile over the ResCheckConcavity() call
But obviously the global view/tile arrangement state has changed to introduce the split.

Obviously there is no SIGSEGV issue with accessing a TiFree() item, as the arena is not unmapped, the allocator is single threaded, the thread that is running has control of the CPU.

So the questions are:

• Does this function perform correctly as-is ? If so why and maybe there is a reason to comment non-obvious (to me!) expectation
• Is it always making a correct branching decision after returning from ResCheckConcavity() in the code immediately following it, using the old information of what the tile looked like before the split/join operation.
• Does this assume the accessed fields are still valid (containing original values) and were not invalidated/overwritten by the split/join operation ?
• Is the target tile of the next iteration correct, or should it see (have visibility of) the newly inserted tile(s) ?

If I can understand this better and if there are genuine concerns here, then I think it is possible to write a custom CodeQL rule that can check for all theoretical paths such as this and confirm that invalidation of data did/did-not occur when it should/should-not have, to check similar code paths like this.

If however it is found to be operating perfectly, doing the correct thing, then the only minor matter maybe to save the restart information from both tiles before ResCheckConcavity() and use the saved data to make the branching decision. Which records the expected semantics of what is going on.

Thanks

---

magic/resis/ResFract.c

Lines 100 to 110 in 705b4da

	/* ok, we may have found a concave corner */
	ResCheckConcavity(resSrTile, resTopTile, tt);
	if (resTopTile == NULL) break;
	if (BOTTOM(resTopTile) != TOP(resSrTile))
	{
	resTopTile = RT(resSrTile);
	}
	else
	{
	resTopTile=BL(resTopTile);
	}

Some initial notes on this:

@@ -100,6 +100,18 @@ enumerate:
 		/* ok, we may have found a concave corner */
 		ResCheckConcavity(resSrTile, resTopTile, tt);
 		if (resTopTile == NULL) break;
+		// VALGRIND ResCheckConcavity() above may have invalidated a tile which might need reloading here?
+		//  ResCheckConcavity->resWalkdown->ResSplitX->TiTileY->TiFree freed
+		//  My guess is the expression below TOP(resSrTile) is the cause when ResSplitX occured
+		//  One strategy maybe to save the next tile position data such as TOP(resSrTile) before calling ResCheckConcavity() above:
+		// VALGRIND invalid read4 Address 0x18ee14ac is 4 bytes before a block of size 8 client-defined
+		// 18ac3000-28ac3000 rw-p 00000000 00:00 0
+		//  TOP(resSrTile) when invalidated is BOTTOM(RT(resSrTile)) is resSrTile->ti_rt->ti_ll.p_y
+		// the reason valgrind reports 4 bytes before a block of 8, is the block of 8 is the ti_client
+		//  used for the TiFree list
+		// a quick read through indicates either of the 2 tiles maybe split, so both sides may need to be addressed
 		if (BOTTOM(resTopTile) != TOP(resSrTile))
 		{
 		    resTopTile = RT(resSrTile);

[dlmiles]

A sketch that should have same behaviour as now but will not trigger use-after-free checks

// save restart information
Tile *resTopTileBl = (resTopTile) ? BL(resTopTile) : 0;  // resTopTile maybe NULL
Tile *resSrTileRt = RT(resSrTile);                       // can use TOP(resSrTileRt)
int resTopTileY = (resTopTile) ? BOTTOM(resTopTile) : 0; // resTopTile maybe NULL
//int resSrTileY = TOP(resSrTile);     // no need to save this as BOTTOM(resSrTileRt) will provide

// ResCheckConcavity()  resSrTile or resTopTile or neither tile maybe Split/Joined underneath us,
// so we saved the restart point above.
ResCheckConcavity(resSrTile, resTopTile, tt); 

if (resTopTile == NULL) break; /* search finished signal */
ASSERT(resSrTileRt, "resSrTileRt");  // assumption

// If we ignore valgrind and test this theory out, these ASSERTs should not fail
//  vvvv update to follow
ASSERT(resTopTileY == BOTTOM(resTopTile), "resTopTileY == BOTTOM(resTopTile)");  // Will trigger (condx does not hold true)
ASSERT(BOTTOM(resSrTileRt) == TOP(resSrTile), "BOTTOM(resSrTileRt) == TOP(resSrTile)");
ASSERT(resSrTileY == TOP(resSrTile), "resSrTileY == TOP(resSrTile)");
ASSERT(resSrTileRt == RT(resSrTile), "resSrTileRt == RT(resSrTile)"); // Will trigger (condx does not hold true)
// ^^^ update to follow
ASSERT(resTopTileBl == BL(resTopTile), "resTopTileBl == BL(resTopTile)");

// perform restart point
if (resTopTileY != BOTTOM(resSrTileRt)) // BOTTOM(resTopTile) != TOP(resSrTile)) 
{ 
    resTopTile = resSrTileRt; // RT(resSrTile); 
} 
else 
{ 
    resTopTile = resTopTileBl; // BL(resTopTile); 
}