Renaming PlayerGui hangs and disables CoreGui

[colbert2677 (ImagineerColbert)]

Reproduction Steps
This can be reproduced by immediately renaming the PlayerGui when the player joins. This works with both Immediate and Deferred SignalBehavior, can be done with any script type (exception: this will not work with a ReplicatedFirst LocalScript) and is not specific to any experience (i.e. it can happen anywhere).

game:GetService("Players").PlayerAdded:Connect(function (player)
    player.PlayerGui.Name = "PlayerGui "
end)

Expected Behavior
CoreGui should be present at all times and should not be able to be influenced by developers aside from exposed APIs SetCore(GuiEnabled).

Actual Behavior
Due to dependencies from the CoreScripts on internal members of PlayerGui, the rename causes a permanent hang because the CoreScripts cannot find PlayerGui and call those internal methods. This results in the CoreGui being disabled.

In-experience result:

Studio output (F9 Developer Console is also disabled with this applied):

Workaround
There’s no developer-facing workaround for this issue besides not renaming PlayerGui (there shouldn’t be a reason to anyhow). For Roblox, it would be not to assume that there is a child named PlayerGui in the Player or to create a PlayerGui property that references the created PlayerGui so it has index precedence. PlayerGui is created with the player and is immediately available like the LocalPlayer to the client so finding it by ClassName instead may also work. This is internal code so I can only offer what I think I know as workarounds but they may not be the answer.

Issue Area: Engine
Issue Type: Other
Impact: Very High
Frequency: Very Rarely
Date First Experienced: 2022-09-18 18:09:00 (-04:00)
Date Last Experienced: 2022-09-18 18:09:00 (-04:00)

[DeodorantSlice37 (ae7100)]

why are you renaming it though

[colbert2677 (ImagineerColbert)]

I was informed that this was a problem by a developer friend of mine. I have no use case for renaming the PlayerGui nor is that relevant to this bug report – the issue should be reported regardless of whether or not there’s a use case for doing it.

[TheNexusAvenger (Nexus)]

This reminds me of bug reports a couple of years ago about renaming services. Using GetService is the recommended way to get around services being renamed, but there isn’t anything for child instances. Renaming PlayerGui breaks so many tools since we can’t do something like WaitForChildOfClass("PlayerGui"). Similar to renaming services, I am not sure which is the bigger problem: that the name isn’t protected and can lead to game-breaking bugs like this or that we assume the name is as-is.

[DeodorantSlice37 (ae7100)]

damn you’re right, roblox just assumes the file will always have the same name, should have it’s own class, or a property (locked for only the folder) to show that it’s the original playergui folder

[cpguy5089]

Some people believe that renaming the services/children of game is an effective anticheat, even though game:GetService("thingy") exists

[MagmaBurnsV]

I like how this actually gives a good reason to add something like WaitForChildWhichIsA, so we don’t have to do some spaghetti coding like the following:

local PlayerGui: PlayerGui? = Player:FindFirstChildWhichIsA("PlayerGui")
	
if not PlayerGui then
	local ChildAdded: RBXScriptConnection; ChildAdded = Player.ChildAdded:Connect(function(Child: Instance)
		if Child:IsA("PlayerGui") then
			PlayerGui = Child
			ChildAdded:Disconnect()
		end
	end)
end

-- VS

local PlayerGui: PlayerGui? = PlayerGui:WaitForChildWhichIsA("PlayerGui")

[WheretIB]

Thank you for the report and an example, we are looking into fixing this.

[colbert2677 (ImagineerColbert)]

FWIW: this is still a bug. I have a repro place on my profile that I orignally dropped the code in to check if it affected live servers; that repro place still emits the same behaviour reported.

It strikes me as surprising that core scripts don’t have any kind of guard for this case considering some of them rely on internal methods of PlayerGui. Developers aren’t allowed to disable a lot of CoreGuis for good reason yet a single line of code can do exactly that.

Seems moderately important, especially in the context of abusing this bug for malicious purposes (impersonating official modals), but I don’t want to backseat triage, rather just call back some attention and provide an update. I’m sure this is a more complex bug to resolve than it appears though, ergo no change since the last staff reply.

[sv_du]

I don’t see how this is hard to fix. The least they can do is disable the renaming of PlayerGui

[colbert2677 (ImagineerColbert)]

I think so too, similarly to how the names of most core instances are locked, but PlayerGui feels like a different uncaught case because it’s a developer-facing instance. I don’t want to make assumptions or backseat engineer on how difficult this is, whether it’s as simple as special casing PlayerGui against certain modifications or if more work needs to go into it.

At the end of the day, the post is only an update that the bug still exists.

[TheRealANDRO (ANDRO)]

I believe this would be a good method of fixing this, I don’t even understand why engineers thought it would be a good idea to get the Instance from its name anyway.

[OnceCrowned]

@colbert2677

Seems moderately important, especially in the context of abusing this bug for malicious purposes (impersonating official modals), but I don’t want to backseat triage, rather just call back some attention and provide an update. I’m sure this is a more complex bug to resolve than it appears though, ergo no change since the last staff reply.

Luckily, the bug does not affect the purchase prompt modal, which is the only CoreGUI modal worth impersonating. The CoreGUI hanging is from one faulty WaitForChild in the emote UI handler.

[BullfrogBait]

I got teleported to an experience with flashing lights and extremely loud sounds that used this bug to stop players from leaving. Glad it’s being actively fixed.

[Hexcede]

@MagmaBurnsV

I like how this actually gives a good reason to add something like WaitForChildWhichIsA , so we don’t have to do some spaghetti coding like the following:

local PlayerGui: PlayerGui? = Player:FindFirstChildWhichIsA("PlayerGui")
	
if not PlayerGui then
	local ChildAdded: RBXScriptConnection; ChildAdded = Player.ChildAdded:Connect(function(Child: Instance)
		if Child:IsA("PlayerGui") then
			PlayerGui = Child
			ChildAdded:Disconnect()
		end
	end)
end

-- VS

local PlayerGui: PlayerGui? = PlayerGui:WaitForChildWhichIsA("PlayerGui")

Alternatively:

local function waitForChildWhichIsA(parent: Instance, className: string)
	while not parent:FindFirstChildWhichIsA(className) do
		parent.ChildAdded:Wait()
	end
	return parent:FindFirstChildWhichIsA(className)
end

@DeodorantSlice37

why are you renaming it though

The main problem is that this can be easily abused by bad actors. Really you shouldn’t want to rename PlayerGui for any particular reason, but the fact that you can and it can be abused in this way is a problem.

@sv_du

I don’t see how this is hard to fix. The least they can do is disable the renaming of PlayerGui

This can break games (even if it is a bad idea to rename the PlayerGui). It would be better to just resolve the issue of the CoreScript code searching for instances by name, instead of changing up the engine to get around an unsafe practice in the CoreScript code.

@TheRealANDRO

I believe this would be a good method of fixing this, I don’t even understand why engineers thought it would be a good idea to get the Instance from its name anyway.

Probably because they don’t really seem to follow any sort of guidelines or reviews process, so it is kind of natural that issues like this will come up at some point. If you look at any of the CoreScripts you can find a TON of issues like this, they just tend to not have breaking impacts or be abusable by developers (and it isn’t localized or anything, it’s a pretty widespread issue in all the CoreScript code). Generally, I have a ton of gripes about the quality of Roblox’s CoreScripts.

---

There are lots of deprecated methods and properties being used, it doesn’t follow a clear style guide (which is perfectly okay but a style guide would help with some of this stuff), there are lots of bugs, race conditions, performance issues, bad practices, etc being used. And the problem just keeps getting worse as more builtin plugins (which can no longer be modified afaik) and CoreScript/CoreGui code are added.

For a brief amount of time, Roblox published their CoreGui code to GitHub. I do really wish they’d bring that back, because I would love to contribute to that. There are genuinely too many issues with the CoreScript code for me to feasibly count. And while most of them are minor, and might generally be acceptable in people’s games, CoreScript code is somewhat sensitive, and it’s basically part of the engine. Any memory leaks and performance issues in CoreScript code aren’t things that developers can fix themselves.

I feel like this particular bug is a good demonstration of that apparent lack of review/guidelines for CoreGui code having consequences. Without good review and good quality control in CoreScript code, we could see even worse issues than this, like privilege escalation in live games as the surface area for abusable bugs grows.

[WheretIB]

Issue from the first post should be fixed for Roblox Menu/F9 now.

There is still an issue with chat being broken by this, will notify the code owners.

[WheretIB]

Follow-up for errors coming from Legacy Chat scripts: we decided not to update those as there is a replacement for legacy chat service that no longer has an issue with renamed PlayerGui.

[ValiantWind (Val)]

This has been an issue for a while. I tried reporting it to @Bug-Support months ago and never got a response.

Glad to see it finally got reported.