PSA: Order of children can't be relied on after a child is moved!

Resources Community Tutorials
,
#1

Preface

This code looks innocent enough, but it might not be.

local tree = workspace:FindFirstChild("Tree")
local branch = tree:FindFirstChild("Branch")
-- We know there is a branch Part in the tree here
local branchColor = branch.Color

The takeaway is this: You should be making separate folders inside an Instance for anything that will ever be moved or destroyed without its parent. If this isn’t doable yet, at least use a Folder for character models.

All credit goes to the original discovery by @jakedies. I made this post only to add visibility, make the recommendation obvious for beginners, highlight the most dangerous interaction, and explicitly mention that indexing is impacted.

Children order has an exception

Children of a parent are normally ordered based on when they were parented to it, with the children parented earlier coming before the ones parented later. This is why indexing workspace.Tree gives you the tree you inserted in Studio, and not the character model of the played named “Tree”. When a parent has more than 20 children and one is moved or destroyed, it is removed through a “fast removal” roughly similar to this Lua code:

local function fastRemove(children, index)
    children[index] = children[#children]
    children[#children] = nil
end

This means that a child parented to some Instance later can be ordered before one that was parented earlier.

This applies everywhere:

  • Indexing thing.Child, thing["Child"]
  • Child returned by thing:FindFirstChild()
  • thing:GetChildren() order
  • thing:GetDescendants() order

Workspace:FindFirstChild(“ChildHere”) and Workspace.ChildHere is the most dangerous

By default, a character model’s name is the player’s name, and it is parented to Workspace. This issue means that if any code moves or destroys any direct child of Workspace, future accesses to a child of Workspace, even one with a different name, can give you a character model instead. This means that bad actors can rename themselves to the names of other Workspace children to cause your game to error.

Demonstration

local firstChild = Instance.new("Model", workspace)

for index = 1, 20 do
	Instance.new("Model", workspace)
end

local targetValue = Instance.new("BoolValue")
targetValue.Name = "TargetValue"
targetValue.Parent = workspace

-- We expect to find targetValue when we do workspace:FindFirstChild("TargetValue"), not this
local fakeTargetValue = Instance.new("Model")
fakeTargetValue.Name = "TargetValue"
fakeTargetValue.Parent = workspace

-- This will cause fakeTargetValue to be swapped in child order with the first model we added
firstChild:Destroy()

-- Changing this to workspace.TargetValue won't help
local accessedInstance = workspace:FindFirstChild("TargetValue")
-- accessedInstance will be fakeTargetValue, and this will error!
local value = accessedInstance.Value

Prospects for this behavior being changed

“Properly” removing a child would be too expensive, so the core behavior here is unlikely to change. Roblox could eliminate the easiest to abuse case by adding a special Folder for characters to be parented under in Workspace, which would also have prevented the close call with the sibling instance order change.

6 Likes
What order do newly created instances take?
#2

Either I don’t understand the post or this isn’t a big problem?

If you have multiple children of the same name then you should know that it is possible that you might not index the right one.

I mean why do you even have two children of the same name in the first place, you really shouldn’t be doing that either way, especially when you want to reindex that child again.

5 Likes
#3

I explicitly made a section for the most dangerous part of this, where a character model can share the same name as a child of Workspace, either by chance or more likely maliciously. This breaks a guarantee many developers rely on. It’s a pretty big problem.

2 Likes
#4

just check which class it is…

:IsA() works fine

1 Like
#5

That’s great except:

  • What you’re really targeting may also be a model.
  • You have to know about this behavior to check for it.
  • That’s not a good solution to the problem. The good solution is described in two lines in the original post.
1 Like
#6

never said use IsA on the model

if you want to see if it is a player you can literally use FindFirstChildWhichIsA() to see if a humanoid exists

if it exists it’s obviously a player or an npc

better yet, GetPlayerFromCharacter() can be used instead if you don’t care about npcs

2 Likes
#7

You wouldn’t write code to check if what you accessed was a player every time, that’s just not how you’d solve this.

#8

from what I got in this post you are saying a player could have the same name as an instance in workspace

please tell me how what I said wouldn’t work, because it does
it is easy to see if a player is a player or not

#9

The issue is that you do not want to have to check whether or not the instance you accessed is a player every single time you access a child of Workspace. It’s a ridiculous solution to a simple problem with an easy solution. A check like that is extremely easy to forget and cause you problems, and it’s wasteful, and it’s bad design.

#10

This is not a huge issue, afaik, no one really writes code that relies upon the order of instances under a model or any other instance. Next, FindFirstChild() as its name suggests, it returns the first instance found with the name provided. So it would be a bad idea to have instances with similar names.

Coming to the exploiter’s problem, they can even add ofc name other instances as Humanoid, or add multiple humanoid instances under the character. The only way to tackle it is by Anti Exploit.

1 Like
#11

Caching is a thing, you can access it once and store it’s reference in a variable.

1 Like
#12

Players:GetPlayerFromCharacter and Player.Character isnt using FindFirstChild. Your logic is flawed. Any real use example?

#13

This problem can easily be fixed using caching and some workarounds. but I agree with you that roblox should take a step further into parenting player characters to their own folder to prevent a lot of headaches, and it’s just more organized, predictable and reliable!

The solution is to make a special folder inside workspace where characters would be parented like PlayerScripts container, etc…

#14

In general whether or not this problem exists you shouldn’t really be relying on this unless you can absolutely ensure that the thing you’re trying to index will not experience a name confliction. You don’t need folders if you structure your code better. Folders are just for organisation’s sake.

Case in OP isn’t a good example because it’s already bad practice, there’s a better way to accomplish that: for example, tag your trees with CollectionService if you need to operate across multiple trees in your experience or use a unique identifier (e.g. with attributes, a different name you know will not conflict later, so on) if you need to pluck a specific tree.

Bad actor character renaming is a non-issue because the change doesn’t propagate to the server; and if it does then that’s a sign that your code structuring is bad enough to allow that to happen or that the engine is replicating something it realistically shouldn’t. Concern yourself only with potential conflictions arising out of legitimate usernames that may have funky conflictions with your experience.

You never really should be relying on child order; and if you do, then you most likely have a very specific way of handling it that’s not dangerous to your code. This is not big, it’s actually pretty trivial.

1 Like
#15

The issue isn’t that this is hard to fix. It’s definitely not. The issue is that this behavior is not intuitive and breaks the way code is traditionally written. There’s nothing wrong with it, it’s just important to be aware of.

I’m referring them to renaming their account.

Tagging is great, and I’d always advocate using it for something like trees. It doesn’t always fit and forcing it in for objects that there should only be one of is awkward. I don’t think it’s a bad solution at all, but I’d avoid a more unpopular recommendation like that in general when a more intuitive one does the job.

I’d disagree with this. It isn’t hard to change your game to fix, but it does require a change for a large portion of games.

Sure, but I don’t think this is a good way to deal with the issue because it’s really easy to miss.

#16

This kind of thing has very rarely been an actual issue that I’ve observed except in some very esoteric cases and they’re usually caught pretty quickly. It’s really a matter of organisation, naming weariness and how you’re writing your code. It’s a trivial matter being given more attention than it actually deserves.

The way the account naming point is referenced in the thread brings the implication that you’re referring to bad actors literally renaming their characters to force code errors. If this is a thread targeted at beginners then their first thought isn’t going to be people spending money to break your game but rather running exploits to rename themselves to throw errors. Don’t clarify to me, clarify to the people you’re writing to.

In terms of tagging, my assumption here is simply that you have many “trees” by taking the example literally and therefore suggesting that you use tags to group your instances instead. It’s pretty popular, good practice and you should use it anyhow. Nothing beats not having the hierarchy as a dependency. That being said, for singular objects obviously you would have to use some form of indexing which is why I mentioned a few times that you will most likely have a very clear way of accessing that actual instance such that it would not cause conflictions with a user’s character.

But sure. All I’m saying is that it’s trivial. I’m not saying the fix proposed in the solution is bad - I do it anyway for convenience’s sake working with characters in at least one project to date - but I simply don’t think it’s necessary or a fit for every game and it’s not that big of a worry. It’s neither hard to fix nor non-trivial a concern. Really, resolve this any way you want.

2 Likes
#17

It’s effectively an easily abusable denial of service attack many games would find themselves vulnerable to. Since it’s so easy to prevent, it’s just a matter of following best practices. The tree example was just the shortest demonstration of something that could break. Signing up for accounts is free. I think the thread was pretty clear. It’s something you realize once, adopt best practices, and never worry about again.

#18

:man_shrugging:

I personally think that calling this best practice is a little far fetched, as is presenting it as a bigger issue than it truly is. To explain what I mean; this doesn’t happen if you don’t fall into being very dependent on the workspace’s contents. In the instance you do, you will typically set up your workspace in such a way that clashing with same-name instances from characters is a non-issue or an extreme edge case.

If the main takeaway is character name conflictions then that’s what the PSA should be more heavily targeted towards in terms of name and content (i.e. “PSA: Character names can conflict with FindFirstChild calls”). The tree might’ve been a simple example, but it was a very poor one in my own opinion because it doesn’t reflect a very realistic scenario that might pop up in production - or maybe I’m just being pedantic on word choice and expressing a solution in terms of that. Guess it wouldn’t make too hard a difference if you used a different name.

Good structures typically never have to even worry about this issue and it’s not as huge an issue for most; I don’t deny that it’s not a plausible issue, but what I do disagree on is calling it a non-trivial issue. Rather than focusing on specifics, you should aim to remove these vectors of unpredictability from your code. Yes, organising characters into a folder is a viable option, but there are likewise options that don’t require changing character parents, like instead hand-replicating content or organising the workspace into folders instead. Thus, I don’t see us agreeing besides our disagreement on the importance of this.

I did have a tinker around in Studio though, so in respect of addressing characters being parented differently, for anyone who’s interested, here’s some code you can deploy:

local Players = game:GetService("Players")

local characterFolder = Instance.new("Folder")
characterFolder.Name = "Characters"
characterFolder.Parent = workspace

local function characterAdded(character)
    task.defer(function ()
        character.Parent = characterFolder
    end)
end

local function playerAdded(player)
    player.CharacterAdded:Connect(characterAdded)
    if player.Character then
        characterAdded(player.Character)
    end
end

Players.PlayerAdded:Connect(playerAdded)
for _, player in ipairs(Players:GetPlayers()) do
    playerAdded(player)
end
1 Like
#19
#20