Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[3.x] Validate RIDs before freeing #55764

Merged
merged 1 commit into from
Aug 5, 2022
Merged

[3.x] Validate RIDs before freeing #55764

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion doc/classes/Navigation2DServer.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,8 @@
<return type="void" />
<argument index="0" name="rid" type="RID" />
<description>
Destroys the given RID.
Destroys an object created by the Navigation2DServer.
[b]Note:[/b] See [method VisualServer.free_rid] for details on how to handle RIDs for freed objects.
</description>
</method>
<method name="map_create" qualifiers="const">
Expand Down
3 changes: 2 additions & 1 deletion doc/classes/NavigationServer.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,8 @@
<return type="void" />
<argument index="0" name="rid" type="RID" />
<description>
Destroys the given RID.
Destroys an object created by the NavigationServer.
[b]Note:[/b] See [method VisualServer.free_rid] for details on how to handle RIDs for freed objects.
</description>
</method>
<method name="map_create" qualifiers="const">
Expand Down
3 changes: 2 additions & 1 deletion doc/classes/Physics2DServer.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -654,7 +654,8 @@
<return type="void" />
<argument index="0" name="rid" type="RID" />
<description>
Destroys any of the objects created by Physics2DServer. If the [RID] passed is not one of the objects that can be created by Physics2DServer, an error will be sent to the console.
Destroys an object created by the Physics2DServer.
[b]Note:[/b] See [method VisualServer.free_rid] for details on how to handle RIDs for freed objects.
</description>
</method>
<method name="get_process_info">
Expand Down
3 changes: 2 additions & 1 deletion doc/classes/PhysicsServer.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -628,7 +628,8 @@
<return type="void" />
<argument index="0" name="rid" type="RID" />
<description>
Destroys any of the objects created by PhysicsServer. If the [RID] passed is not one of the objects that can be created by PhysicsServer, an error will be sent to the console.
Destroys an object created by the PhysicsServer.
[b]Note:[/b] See [method VisualServer.free_rid] for details on how to handle RIDs for freed objects.
</description>
</method>
<method name="generic_6dof_joint_get_flag">
Expand Down
14 changes: 13 additions & 1 deletion doc/classes/VisualServer.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -961,7 +961,19 @@
<return type="void" />
<argument index="0" name="rid" type="RID" />
<description>
Tries to free an object in the VisualServer.
Destroys an object created by the VisualServer. If the [RID] passed is not one created by the server that created it (e.g. VisualServer, PhysicsServer, etc.), an error will be sent to the console.
[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 [code]RID()[/code] or any other valid RID.
[codeblock]
var r: RID = VisualServer.get_test_cube()
VisualServer.free_rid(r)
print("ID: ", r.get_id()) # It is not safe to access or free an invalid RID
r = RID() # Reset the RID so it is safe to use again.
print("ID: ", r.get_id())

# Output:
# ID: 157 # Freed RID has invalid data
# ID: 0 # RID has been properly reset
[/codeblock]
</description>
</method>
<method name="get_render_info">
Expand Down
9 changes: 8 additions & 1 deletion modules/bullet/bullet_physics_server.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1480,6 +1480,11 @@ bool BulletPhysicsServer::generic_6dof_joint_get_flag(RID p_joint, Vector3::Axis
}

void BulletPhysicsServer::free(RID p_rid) {
if (!p_rid.is_valid()) {
ERR_FAIL_MSG("Invalid RID.");
return;
}

if (shape_owner.owns(p_rid)) {
ShapeBullet *shape = shape_owner.get(p_rid);

Expand All @@ -1490,6 +1495,7 @@ void BulletPhysicsServer::free(RID p_rid) {

shape_owner.free(p_rid);
bulletdelete(shape);

} else if (rigid_body_owner.owns(p_rid)) {
RigidBodyBullet *body = rigid_body_owner.get(p_rid);

Expand Down Expand Up @@ -1532,8 +1538,9 @@ void BulletPhysicsServer::free(RID p_rid) {
space_set_active(p_rid, false);
space_owner.free(p_rid);
bulletdelete(space);

} else {
ERR_FAIL_MSG("Invalid ID.");
ERR_FAIL_MSG("Invalid RID.");
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

}

Expand Down
6 changes: 5 additions & 1 deletion modules/navigation/godot_navigation_server.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -427,6 +427,10 @@ COMMAND_4(agent_set_callback, RID, p_agent, Object *, p_receiver, StringName, p_
}

COMMAND_1(free, RID, p_object) {
if (!p_object.is_valid()) {
ERR_FAIL_MSG("Invalid RID.");
return;
}
if (map_owner.owns(p_object)) {
NavMap *map = map_owner.get(p_object);

Expand Down Expand Up @@ -473,7 +477,7 @@ COMMAND_1(free, RID, p_object) {
memdelete(agent);

} else {
ERR_FAIL_COND("Invalid ID.");
ERR_FAIL_COND("Invalid RID.");
}
}

Expand Down
10 changes: 9 additions & 1 deletion servers/physics/physics_server_sw.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1199,6 +1199,11 @@ bool PhysicsServerSW::generic_6dof_joint_get_flag(RID p_joint, Vector3::Axis p_a
}

void PhysicsServerSW::free(RID p_rid) {
if (!p_rid.is_valid()) {
ERR_FAIL_MSG("Invalid RID.");
return;
}

_update_shapes(); //just in case

if (shape_owner.owns(p_rid)) {
Expand All @@ -1211,6 +1216,7 @@ void PhysicsServerSW::free(RID p_rid) {

shape_owner.free(p_rid);
memdelete(shape);

} else if (body_owner.owns(p_rid)) {
BodySW *body = body_owner.get(p_rid);

Expand Down Expand Up @@ -1247,6 +1253,7 @@ void PhysicsServerSW::free(RID p_rid) {

area_owner.free(p_rid);
memdelete(area);

} else if (space_owner.owns(p_rid)) {
SpaceSW *space = space_owner.get(p_rid);

Expand All @@ -1261,6 +1268,7 @@ void PhysicsServerSW::free(RID p_rid) {

space_owner.free(p_rid);
memdelete(space);

} else if (joint_owner.owns(p_rid)) {
JointSW *joint = joint_owner.get(p_rid);

Expand All @@ -1271,7 +1279,7 @@ void PhysicsServerSW::free(RID p_rid) {
memdelete(joint);

} else {
ERR_FAIL_MSG("Invalid ID.");
ERR_FAIL_MSG("Invalid RID.");
}
};

Expand Down
9 changes: 8 additions & 1 deletion servers/physics_2d/physics_2d_server_sw.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1121,6 +1121,11 @@ Physics2DServer::JointType Physics2DServerSW::joint_get_type(RID p_joint) const
}

void Physics2DServerSW::free(RID p_rid) {
if (!p_rid.is_valid()) {
ERR_FAIL_MSG("Invalid RID.");
return;
}

_update_shapes(); // just in case

if (shape_owner.owns(p_rid)) {
Expand Down Expand Up @@ -1169,6 +1174,7 @@ void Physics2DServerSW::free(RID p_rid) {

area_owner.free(p_rid);
memdelete(area);

} else if (space_owner.owns(p_rid)) {
Space2DSW *space = space_owner.get(p_rid);

Expand All @@ -1181,14 +1187,15 @@ void Physics2DServerSW::free(RID p_rid) {
free(space->get_default_area()->get_self());
space_owner.free(p_rid);
memdelete(space);

} else if (joint_owner.owns(p_rid)) {
Joint2DSW *joint = joint_owner.get(p_rid);

joint_owner.free(p_rid);
memdelete(joint);

} else {
ERR_FAIL_MSG("Invalid ID.");
ERR_FAIL_MSG("Invalid RID.");
}
};

Expand Down
7 changes: 7 additions & 0 deletions servers/visual/visual_server_raster.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,11 @@ void VisualServerRaster::_draw_margins() {
/* FREE */

void VisualServerRaster::free(RID p_rid) {
if (!p_rid.is_valid()) {
ERR_FAIL_MSG("Invalid RID.");
return;
}

if (VSG::storage->free(p_rid)) {
return;
}
Expand All @@ -80,6 +85,8 @@ void VisualServerRaster::free(RID p_rid) {
if (VSG::scene_render->free(p_rid)) {
return;
}

ERR_FAIL_MSG("Invalid RID.");
}

/* EVENT QUEUING */
Expand Down