Fix out of bounds access into empty image data

[nmrkr]

Fixes #46189.

This issue only affects X11.

Cherry-picking this does not seem possible due to restructuring. I have submitted #46249 to apply this fix to the 3.2 branch.

[akien-mga]

I think the check should be earlier, likely together with if (p_icon.is_valid()). There's no reason to do all the processing above for an invalid icon.

[nmrkr]

This was also my first idea. But then setting an empty Ref would be semantically equivalent to setting a valid but empty image. I am not familiar enough with X11 to say wether or not calling XDeleteProperty on net_wm_icon is actually the same as calling XChangeProperty with an empty image. I see no difference in behaviour on my particular setup, but I didn't want to remove the distinction between the two code paths without being sure.

This function is unlikely to be called at a high enough frequency for the performance difference to matter, but I'm also not invested in this particular solution and will gladly change it to an earlier exit. Your call :)

[pouleyKetchoupp]

It should be fine to call XDeleteProperty for en empty image.

Actually, in order to make it consistent with other platforms, an invalid image could be handled with ERR_FAIL_COND(!p_icon.is_valid()); so it would just raise an error, and the proper way to set no icon would be to use an empty image.

[nmrkr]

Consistency across all platforms sounds like a very good goal. Should this then be moved into a separate issue?

In any case, here's the situation as it currently is in master:

	X11	Win	UWP	OSX	JS
Empty Ref	Remove Icon	ERR_FAIL_COND	Silently does nothing	Crash	ERR_FAIL_COND
Empty Image	Crash	Use Empty Image as Icon	Silently does nothing	ERR_FAIL_COND	Use Empty Image as Icon

Looking at the other platforms actually revealed another bug. OSX dereferences the Ref unchecked and crashes on set_icon(null).

I guess there should be an executive decision on what The Right Thing™ to do is for each case.

[pouleyKetchoupp]

I think it should be safe to say the current behavior on Windows is the way to go accross platforms, but let's see if there's a consensus in @godotengine/_platforms.

The current PR could address it completely on X11 by removing the icon on empty image and error on empty ref. OSX needs to be addresses in a separate PR.

[akien-mga]

Yeah I think the Windows/JS behavior is good and should be used for all platforms.

[YuriSizov]

This PR needs a rebase and to address the discussion above.

[akien-mga]

Superseded by #78437.