[3.x] Validate RIDs before freeing

[TokisanGames]

Background

It is possible for a class to free objects in the visual server and physics servers that it doesn't own by sending RIDs with dangling pointers to VS::free(). It has been happening in C++ and should also be possible in GDscript and other languages. Refer to PR #54650 and issue #53374 .

Changes

• Introduces a validity check for RIDs in VisualServer::free() and the three physics servers Bullet, SW, 2DSW, and NavigationServer, Navigation2DServer. However, this cannot protect against RIDs with dangling pointers by design of the RID system.

• Adds a warning to the documentation for VisualServer, PhysicsServer, Physics2DServer, NavigationServer, and Navigation2DServer to alert users to clear their RIDs before reusing them.

• Matches VisualServer::free(), NavigationServer::free() behavior to that of Physics(2D)Server::free(). When VS::free() receives an invalid RID, it now prints an error message to the console, as the Physics servers do.

As a result of the third, the engine dumps 30-50 errors to the console complaining about invalid RIDs sent to VS::free(). These errors are fixed by #54650.

@lawnjelly @RandomShaper

[TokisanGames]

@lawnjelly Can we get this reviewed? This is something that should be pushed into 3.5.
@akien-mga

[lawnjelly]

Would need discussion with @reduz when he is available again I think (there are a number of ways of doing this, and I don't know offhand whether a NULL rid should generate an error, it could be used by e.g. dummy servers).

[lawnjelly]

Been a while, but looking at the logic, will the owns() calls return false already and thus get to the INVALID_ID part already?

	_FORCE_INLINE_ bool owns(const RID &p_rid) const {
		if (p_rid.get_data() == nullptr) {
			return false;
		}

This seems to be the same as is_valid() check:

	_FORCE_INLINE_ bool is_valid() const { return _data != nullptr; }

[akien-mga]

Suggested change

	Note: After freeing the object, the RID now has a reference to invalid memory. It is not safe to free an invalid RID. Make sure to reset it before using it again by assigning it to `RID()`.
	[b]Note:[/b] After freeing the object, the RID now has a reference to invalid memory. It is not safe to free an invalid RID. Make sure to reset it before using it again by assigning it a new RID.

[TokisanGames]

I specifically meant assigning it to RID() as shown in the example shown in the xml.
eg.

VisualServer.free(myrid)
myrid = RID()

[akien-mga]

Yeah but that's not portable syntax (won't work in C#), so I suggested the rephrasing to work it around. Isn't "a new RID" explicit enough? Also, wouldn't it work to assign a new valid RID from another object, if that's what the user wants?

[akien-mga]

But if you want to keep the explicit RID() reference I don't mind. Just note the [b] formatting around Note: which should be added, we use it everywhere.

[TokisanGames]

Assigning it to a new RID to me suggests the meaning, "assigning it to another valid RID you have from another object, or creating a new RID (as part of a new object)". What we specifically want is to re-initialize the variable to a null or zero state.

The example shown is in GDScript. Aren't all document examples in gdscript? We could put in a C# version, but this would be the first time I've seen C# going into the manual. I guess the tutorials have both.

I've done the [b] formatting. I'll leave the RID() until there is a better alternative.

[TokisanGames]

I'm changing it to:

[b]Note:[/b] After freeing the object, the RID now has a reference to invalid memory. It is not safe to use or free an invalid RID. Before using the RID again, make sure to assign it to `RID()` or any other valid RID.
[codeblock]
...

[akien-mga]

This should also be changed in the new NavigationServer:

modules/navigation/godot_navigation_server.cpp
476:            ERR_FAIL_COND("Invalid ID.");

@reduz says:

<Akien> I guess its an ok workaround
not super familiar how this worked

So if @lawnjelly approves, I think this would be good to go (aside from my above review comments). Seems fairly safe either way, one difference for physics servers is that it will no longer run _update_shape(), but that might be fine.

[lawnjelly]

For the case of returning an error message from VisualServerRaster::free() if the RID is NULL, it would be nice to have some confirmation that this can't spam errors in some situations.

Maybe the author can run a headless server build just to test and confirm that this does not spam errors (as dummy servers can sometimes create and free NULL RIDs afaik). If it does spam errors with NULL RID, than maybe it could be changed to return with no error output, or have the output only in the builds where this might be relevant.

[TokisanGames]

For the case of returning an error message from VisualServerRaster::free() if the RID is NULL, it would be nice to have some confirmation that this can't spam errors in some situations.

It should spam errors if there are bugs in the engine that are causing the spam, so they can be identified in beta and rc builds and fixed. The lack of errors and validation is the reason the whole RID system has taken months of our time troubleshooting. It's still a poor system and we're just slapping bandaids on it.

However this section of the PR is not about causing error spamming or not. It is about making the behavior between PhysicsServer, VisualServer, etc the same. Currently PS and VS behave differently. I brought VS inline with PS. The only question was if the opposite was desireable, PS inline with VS and silently fail.

Maybe the author can run a headless server build just to test and confirm that this does not spam errors (as dummy servers can sometimes create and free NULL RIDs afaik). If it does spam errors with NULL RID, than maybe it could be changed to return with no error output, or have the output only in the builds where this might be relevant.

I will try a server build. It appears that will build only on linux?

[akien-mga]

I will try a server build. It appears that will build only on linux?

Linux or macOS only yeah.

[Calinou]

I will try a server build. It appears that will build only on linux?

In 3.x, platform=server builds can only be compiled for Linux and macOS, not Windows.

In master, platform=server was removed in favor of a --headless command line argument that works on all platforms.

[TokisanGames]

Seems fairly safe either way, one difference for physics servers is that it will no longer run _update_shape(), but that might be fine.

Are you referring to _update_shapes() here?
https://github.com/godotengine/godot/blob/1509e9663dd9faa5751a2660c16863b47cc67f1e/servers/physics/physics_server_sw.cpp#L1207

That will only be a problem if the end user of the server (typically another portion of the engine rather than a game dev) is abusing free(). They should not be doing so anyway, and may get an error message to warn them of it. I don't see this as an issue.

I noted that Navigation(2D)Server::free_rid() are const and VisualServer, Physics*Server are not.

[TokisanGames]

I built and ran the godot server. Am I supposed to have a project or something, or is this all you wanted to see, @lawnjelly ?

$ bin/godot_server.x11.opt.tools.64 -v
Godot Engine v3.5.beta.custom_build.858a8043f - https://godotengine.org
 
CORE API HASH: 13445212471483651688
EDITOR API HASH: 2255026694960606884
Loading resource: /home/cory/.config/godot/editor_settings-3.tres
EditorSettings: Load OK!
Loaded builtin certs

I also ran through --test string, math, oa_hash_map, etc. None of these produced any RID errors.

I have pushed an update with the navserver update and doc changes.

[lawnjelly]

Perhaps if the message "Invalid RID" is now being used for a NULL RID with the first check, these messages at the end could be something like "RID not found."? 🤔

That might be handy to differentiate the two cases (although I doubt this later case will happen often).

[lawnjelly]

I built and ran the godot server. Am I supposed to have a project or something, or is this all you wanted to see, @lawnjelly ?

To be honest I don't know much about the server builds. But what I have seen is that with dummy servers it can return NULL RIDs when you e.g. request a new e.g. sky.

See https://github.com/godotengine/godot/blob/3.x/drivers/dummy/rasterizer_dummy.h for examples.

So if we are using dummy servers that return NULL RIDs, we are relying on the client code checking they are valid before freeing, otherwise we will presumably get these error messages. But maybe we will not know whether this is a problem or not until merged and tried in various permutations.

I guess as long as we have done as much testing as we can, it is ok to merge, although it might be an idea to change the two error messages as in my comment above to differentiate the two cases. 👍

[akien-mga]

This went under the radar again, sorry for the delay. Giving it an approval so that we can properly keep track of it and merge.

It's a bit too late for 3.5 now as we're almost releasing 3.5-stable, but we can merge afterwards for 3.6 and consider a cherry-pick for 3.5.x if no regressions are noticed in 3.x.

[akien-mga]

Thanks!