godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
90.59k stars 21.1k forks source link

GodotExtension:memdelete StreamPeerTLS and StreamPeerTCP and HTTPClient will cause corruption #98575

Closed chenjie199234 closed 2 hours ago

chenjie199234 commented 2 hours ago

Tested versions

4.3 stable

System information

windows 11 Godot 4.3.stable

Issue description

program corruption,debug stopped Unexpected

Steps to reproduce

here is the c++ code in GodotExtension

auto tcppeer = memnew(StreamPeerTCP);
auto tlspeer = memnew(StreamPeerTLS);
tcppeer->connect_to_host();
tlspeer->connect_to_stream(tcppeer,"xxhost name");
tlspeer->disconnect_from_stream();
tcppeer->disconnect_to_host();//corruption happened here
auto tcppeer = memnew(StreamPeerTCP);
auto tlspeer = memnew(StreamPeerTLS);
tcppeer->connect_to_host();
tlspeer->connect_to_stream(tcppeer,"xxhost name");
memdelete(tlspeer);
memdelete(tcppeer);//corruption happened here
auto tcppeer = memnew(StreamPeerTCP);
auto httpclient = memnew(HTTPClient);
tcppeer->connect_to_host();
httpclient->set_connection(tcppeer);
memdelete(tcppeer);
memdelete(httpclient);//corruption happened here

Minimal reproduction project (MRP)

code above

AThousandShips commented 2 hours ago

You shouldn't use memdelete for these, they are RefCounted, instead use Ref<StreamPeerTCP> tcppeer = memnew(StreamPeerTCP); and do not delete it, you should just use scope for it

chenjie199234 commented 2 hours ago

@AThousandShips new without delete? will it cause a memory leak

AThousandShips commented 2 hours ago

No because it's a reference counted type, it's protected by scope

chenjie199234 commented 2 hours ago

@AThousandShips thanks.i think it works like c++'s new and delete. but the disconnect is still wrong. i can't disconnect on tcp and tls peer together

AThousandShips commented 2 hours ago

Please fix your code to use the correct types and upload a minimal project to test this

chenjie199234 commented 2 hours ago

@AThousandShips tested by myself use memnew with Ref,works fine. sorry,i think the memnew is like the c++'s new

AThousandShips commented 2 hours ago

It is like new but this type is reference counted, like std::shared_ptr