Fix ImageDataUtil.copyPixels crash on Hashlink

[ACrazyTown]

On C++ when a passed Vector2 instance is null, it'll just make it (0, 0) on the C++ side:

lime/project/src/math/Vector2.cpp

Lines 31 to 41 in 0b08e7f

	if (!val_is_null (vec)) {
	x = val_number (val_field (vec, id_x));
	y = val_number (val_field (vec, id_y));
	} else {
	x = 0;
	y = 0;
	}

Unlike C++, on Hashlink there's no failsafe for if the Vector2 instance is null, it'll just pass it further to ImageDataUtil::copyPixels causing an access violation. This PR checks if we're passing in a null Vector2 on the Haxe side, and makes a new object if we are. I assume this is better than making it on the C++ side but please correct me if I'm wrong

Fixes: openfl/openfl#2754

[joshtynjala]

Both hxcpp and HashLink share Lime's C++ code, so I would prefer if null checks for both targets were consistently all in C++ or all in Haxe.

Looking at some other C++ code in Lime, I suspect that a null pointer will be 0 for HashLink.

Maybe something like this is all that's needed?

if (vec && !val_is_null (vec)) { 

[ACrazyTown]

I'm confused because Lime only seems to make C++ objects when compiling for C++/native:

lime/project/src/ExternalInterface.cpp

Lines 2035 to 2042 in 0b08e7f

	} else {
	Image _alphaImage = Image (alphaImage);
	Vector2 _alphaPoint = Vector2 (alphaPoint);
	ImageDataUtil::CopyPixels (&_image, &_sourceImage, &_sourceRect, &_destPoint, &_alphaImage, &_alphaPoint, mergeAlpha);
	}

When targeting Hashlink it just directly passes the arguments from Haxe:

lime/project/src/ExternalInterface.cpp

Lines 2047 to 2059 in 0b08e7f

	HL_PRIM void HL_NAME(hl_image_data_util_copy_pixels) (Image* image, Image* sourceImage, Rectangle* sourceRect, Vector2* destPoint, Image* alphaImage, Vector2* alphaPoint, bool mergeAlpha) {
	if (!alphaImage) {
	ImageDataUtil::CopyPixels (image, sourceImage, sourceRect, destPoint, NULL, NULL, mergeAlpha);
	} else {
	ImageDataUtil::CopyPixels (image, sourceImage, sourceRect, destPoint, alphaImage, alphaPoint, mergeAlpha);
	}
	}

Where would your fix go? I thought about checking if the pointer was null on the C++ side and then making an object there but I'm not completely sure if that has any negative effect? Would that have to get manually freed since it was made on the C++ side?

[joshtynjala]

If Vector2.cpp isn't the correct place, that's totally fine. I made sure to say "Maybe something like" to be clear that I was not suggesting exactly what you need to do. I haven't worked on that particular code, so you are probably more familiar with how it works than I am at this moment.

Perhaps you could add a check in hl_image_data_util_copy_pixels. Maybe something like this:

if (!destPoint) {

     destPoint = Vector2(0, 0);

}

[joshtynjala]

Note: I don't frequently work with C++, and I did not try that code through a compiler. As before, it's more to give you an idea of where to look, and you can fiddle with getting the right syntax.

I just realized that it's probably a pointer, so maybe more like:

if (!destPoint) {

     destPoint = new Vector2(0, 0);

}

And then you probably need to delete it after, if it wasn't passed in from Haxe.

[ACrazyTown]

If Vector2.cpp isn't the correct place, that's totally fine. I made sure to say "Maybe something like" to be clear that I was not suggesting exactly what you need to do.

Sorry, I wasn't exactly sure what you meant so just wanted to clarify.

I pushed a commit where the function will now allocate a temporary Vector2 object (this is what the hxcpp version does as well) that gets used if the passed Vector2* from Haxe is null. It'll get automatically cleaned up when it goes out of scope.
This is a bit more simpler than managing pointers

[joshtynjala]

That looks like we're on the right track. If I may suggest one last tweak, I think that we should avoid allocating tempPoint when it's not needed.

This should probably work:

Vector2* _alphaPoint = alphaPoint;

if (!_alphaPoint) {

	Vector2 tempPoint = Vector2(0, 0);
	_alphaPoint = &tempPoint;

}

[ACrazyTown]

Should be good to go now

[trnzk]

This kind of usage can cause issues because you’re assigning the address of a local variable (tempPoint) to a pointer. Once the block ends, the local variable goes out of scope, leaving the pointer pointing to invalid memory, which can lead to undefined behavior.

[player-03]

I'd be worried about that too.

[ACrazyTown]

This kind of usage can cause issues because you’re assigning the address of a local variable (tempPoint) to a pointer. Once the block ends, the local variable goes out of scope, leaving the pointer pointing to invalid memory, which can lead to undefined behavior.

So should I just set the pointer to nullptr? Or alternatively do something like this:

if (!alphaImage) { 

    ImageDataUtil::CopyPixels (image, sourceImage, sourceRect, destPoint, NULL, NULL, mergeAlpha); 

} else { 

    if (!alphaPoint) {

        Vector2 tempAlphaPoint = Vector2(0, 0);
        ImageDataUtil::CopyPixels (image, sourceImage, sourceRect, destPoint, alphaImage, &tempAlphaPoint, mergeAlpha); 

    } else {

        ImageDataUtil::CopyPixels (image, sourceImage, sourceRect, destPoint, alphaImage, alphaPoint, mergeAlpha); 

    }

}
 	

[joshtynjala]

Or alternatively do something like this

That seems good to me.

[trnzk]

I think it was fine except for the last commit. Vector2 tempPoint(0, 0); uses the stack and I believe it has negligible overhead considering what the function does.

[ACrazyTown]

Do I roll back to 824da26 or do I proceed with the suggestion I made earlier? 😅 Not sure where to go from here...

[joshtynjala]

Go with this one:

if (!alphaImage) { 

    ImageDataUtil::CopyPixels (image, sourceImage, sourceRect, destPoint, NULL, NULL, mergeAlpha); 

} else { 

    if (!alphaPoint) {

        Vector2 tempAlphaPoint = Vector2(0, 0);
        ImageDataUtil::CopyPixels (image, sourceImage, sourceRect, destPoint, alphaImage, &tempAlphaPoint, mergeAlpha); 

    } else {

        ImageDataUtil::CopyPixels (image, sourceImage, sourceRect, destPoint, alphaImage, alphaPoint, mergeAlpha); 

    }

}