Bf 696 socket#697
Merged
Merged
Conversation
Reviewer's GuideRefactors UDP handling to use GLib's GSocket/GIO infrastructure instead of raw UDP file descriptors, updates UdpDataService to accept GSocketAddress peers, propagates the new socket usage through CoreThread APIs and tests, and wires in gio-2.0 as a build dependency. Sequence diagram for UDP message reception with GSocketsequenceDiagram
participant Sender as SenderHost
participant GSocket as GSocket
participant GSource as GLibSource
participant UdpThread as UdpThread
participant UdpCb as udpThreadCb
participant Ops as UdpThreadOps
participant Service as UdpDataService
activate GSocket
Sender->>GSocket: UDP datagram
GSocket-->>GSource: socket ready (G_IO_IN)
GSource->>UdpCb: callback udpThreadCb(UdpThread)
activate UdpCb
UdpCb->>GSocket: g_socket_receive_from(socket, peer, buf, size, NULL, error)
alt receive error
GSocket-->>UdpCb: n < 0 and error
UdpCb->>UdpCb: log g_socket_receive_from failed
UdpCb-->>GSource: TRUE
else zero bytes
GSocket-->>UdpCb: n == 0
UdpCb->>GSocket: g_object_unref(peer)
UdpCb-->>GSource: TRUE
else valid data
GSocket-->>UdpCb: n > 0, peer, buf
UdpCb->>UdpCb: terminate buf with '\0'
UdpCb->>Ops: on_new_msg(udpThread, peer, buf, n)
activate Ops
Ops->>Service: process(peer, buf, n)
activate Service
Service->>Service: extract port and IP from peer
Service->>Service: process(in_addr, port, buf, n, true)
Service-->>Ops: std::unique_ptr~UdpData~
deactivate Service
Ops-->>UdpCb: bool
deactivate Ops
UdpCb->>GSocket: g_object_unref(peer)
UdpCb-->>GSource: TRUE
end
deactivate UdpCb
Class diagram for updated UDP socket handlingclassDiagram
class CoreThread {
+int tcpSock
+int getUdpSock()
+bool start()
+bool bind_iptux_port()
+void SendNotifyToAll(CoreThread pcthrd)
+void sendFeatureData(PPalInfo pal)
+void SendMyIcon(PPalInfo pal, istream iss)
+void SendDetectPacket(const string ipv4)
+void SendDetectPacket(in_addr ipv4)
+bool SendAskSharedWithPassword(const PalKey palKey, const string password)
+void SendUnitMessage(const PalKey palKey, uint32_t opttype, const string message)
+void SendGroupMessage(const PalKey palKey, const string message)
-std::shared_ptr<ProgramData> programData
-std::shared_ptr<IptuxConfig> config
-mutable std::mutex mutex
-std::unique_ptr<Impl> pImpl
}
class CoreThread_Impl {
+UdpThread* udpThread
+CoreThreadErr lastErr
+GSocket* udpSocket
+std::unique_ptr<UdpDataService> udp_data_service
+bool debugDontBroadcast
+std::list<PPalInfo> pallist
+~Impl()
}
class UdpThreadOps {
+bool (*on_new_msg)(UdpThread udpThread, GSocketAddress peer, const char msg, size_t size)
+void (*on_init_failed)(UdpThread udpThread)
+void (*on_loop_stopped)(UdpThread udpThread)
}
class UdpThread {
+GSocket* socket
+void* data
+const UdpThreadOps* ops
+atomic~UdpThreadState~ state
+GMainLoop* loop
+GThread* thread
+GMainContext* ctx
+guint id
+UdpThread()
+~UdpThread()
}
class UdpDataService {
+UdpDataService(CoreThread coreThread)
+std::unique_ptr<UdpData> process(GSocketAddress peer, const char buf, size_t size)
+std::unique_ptr<UdpData> process(in_addr ipv4, int port, const char buf, size_t size)
}
class GSocket {
+gboolean g_socket_set_broadcast(gboolean broadcast)
+gboolean g_socket_bind(GSocketAddress address, gboolean allow_reuse, GError error)
+gssize g_socket_receive_from(GSocketAddress peer, void buffer, gsize size, GCancellable cancellable, GError error)
+int g_socket_get_fd()
}
class GSocketAddress
class GInetSocketAddress
class GInetAddress
CoreThread --> CoreThread_Impl : has
CoreThread_Impl --> UdpThread : owns
CoreThread_Impl --> UdpDataService : uses
CoreThread_Impl --> GSocket : owns udpSocket
UdpThread --> UdpThreadOps : uses
UdpThread --> GSocket : uses socket
UdpThreadOps --> UdpDataService : on_new_msg calls process
UdpDataService --> CoreThread : holds reference
UdpDataService --> GSocketAddress : process peer
UdpDataService --> GInetSocketAddress : casts peer
UdpDataService --> GInetAddress : extracts IP
GInetSocketAddress --> GSocketAddress : inherits
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The new GIO-based code leaks several GObjects/strings:
anyandbind_addrinbind_iptux_portand theipstring fromg_inet_address_to_stringinUdpDataService::process(GSocketAddress*,...)are never freed/unref’d; consider adding correspondingg_object_unref/g_freecalls on both success and error paths. - In
udpThreadCb,g_socket_receive_fromis called with&errorbut the code unconditionally dereferenceserrorwhenn < 0; it would be safer to check forerror != nullptr(and handle theNULLcase) before logging to avoid a potential null dereference. - Now that the UDP socket is managed as a
GSocket(udpSocketinImpl), please double-check the lifetime and shutdown path (e.g., inClearSublayerandImpl’s destructor) so that the UDP socket is explicitly shut down/unref’d at the appropriate time rather than relying only on destruction.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new GIO-based code leaks several GObjects/strings: `any` and `bind_addr` in `bind_iptux_port` and the `ip` string from `g_inet_address_to_string` in `UdpDataService::process(GSocketAddress*,...)` are never freed/unref’d; consider adding corresponding `g_object_unref`/`g_free` calls on both success and error paths.
- In `udpThreadCb`, `g_socket_receive_from` is called with `&error` but the code unconditionally dereferences `error` when `n < 0`; it would be safer to check for `error != nullptr` (and handle the `NULL` case) before logging to avoid a potential null dereference.
- Now that the UDP socket is managed as a `GSocket` (`udpSocket` in `Impl`), please double-check the lifetime and shutdown path (e.g., in `ClearSublayer` and `Impl`’s destructor) so that the UDP socket is explicitly shut down/unref’d at the appropriate time rather than relying only on destruction.
## Individual Comments
### Comment 1
<location> `src/iptux-core/internal/UdpDataService.cpp:26-29` </location>
<code_context>
+ GInetSocketAddress* isa = G_INET_SOCKET_ADDRESS(peer);
+ guint16 port = g_inet_socket_address_get_port(isa);
+ GInetAddress* ia = g_inet_socket_address_get_address(isa);
+ char* ip = g_inet_address_to_string(ia);
+
+ // TODO: too many conversions, optimize it
+ in_addr ipv4 = inAddrFromString(ip);
+ return process(ipv4, port, buf, size, true);
+}
</code_context>
<issue_to_address>
**issue (bug_risk):** The string from g_inet_address_to_string is leaked and should be freed.
`g_inet_address_to_string()` returns a heap-allocated string that must be freed with `g_free()`. Here, `ip` is leaked on every call. Free it after `inAddrFromString(ip)`:
```c++
char* ip = g_inet_address_to_string(ia);
in_addr ipv4 = inAddrFromString(ip);
g_free(ip);
return process(ipv4, port, buf, size, true);
```
</issue_to_address>
### Comment 2
<location> `src/iptux-core/CoreThread.cpp:468-471` </location>
<code_context>
- if (::bind(udpSock, (struct sockaddr*)&addr, sizeof(addr)) == -1) {
- int ec = errno;
+ GInetAddress* any = g_inet_address_new_from_string(bind_ip.c_str());
+ GSocketAddress* bind_addr = g_inet_socket_address_new(any, port);
+
+ if (!g_socket_bind(pImpl->udpSocket, bind_addr, TRUE, &error)) {
close(tcpSock);
- close(udpSock);
</code_context>
<issue_to_address>
**issue (bug_risk):** GInetAddress and GSocketAddress instances are leaked and error paths don’t clean up the UDP socket.
The GInetAddress from g_inet_address_new_from_string() and the GSocketAddress from g_inet_socket_address_new() are never unreffed, and on the UDP bind failure path the GSocket is also not unreffed, leaking both memory and the socket handle.
You can structure this so all objects are released on both success and error paths, for example:
```c++
GInetAddress* any = g_inet_address_new_from_string(bind_ip.c_str());
if (!any) {
LOG_ERROR("Invalid bind IP: %s", bind_ip.c_str());
close(tcpSock);
g_object_unref(pImpl->udpSocket);
pImpl->lastErr = CORE_THREAD_ERR_UDP_BIND_FAILED;
return false;
}
GSocketAddress* bind_addr = g_inet_socket_address_new(any, port);
g_object_unref(any);
if (!g_socket_bind(pImpl->udpSocket, bind_addr, TRUE, &error)) {
g_object_unref(bind_addr);
close(tcpSock);
g_object_unref(pImpl->udpSocket);
auto errmsg = stringFormat(
_("Fatal Error!! Failed to bind the UDP port(%s:%d)!\n%s"),
bind_ip.c_str(), port, error->message);
g_error_free(error);
LOG_ERROR("%s", errmsg.c_str());
pImpl->lastErr = CORE_THREAD_ERR_UDP_BIND_FAILED;
return false;
}
g_object_unref(bind_addr);
```
Any equivalent pattern that reliably unrefs `any`, `bind_addr`, and `udpSocket` on all paths works.
</issue_to_address>
### Comment 3
<location> `src/iptux-core/CoreThread.cpp:541-544` </location>
<code_context>
- shutdown(udpSock, SHUT_RDWR);
}
int CoreThread::getUdpSock() const {
- return udpSock;
+ // TODO: should no longer use it
+ return g_socket_get_fd(pImpl->udpSocket);
}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** getUdpSock() now relies on a GSocket but doesn’t handle a null or closed socket case.
Because `getUdpSock()` now calls `g_socket_get_fd(pImpl->udpSocket)`, it assumes `udpSocket` is always valid. If this is called during teardown or before `bind_iptux_port()` succeeds, `udpSocket` could be null or closed and `g_socket_get_fd` may misbehave. Consider guarding and returning an invalid fd instead:
```c++
if (!pImpl->udpSocket)
return -1;
return g_socket_get_fd(pImpl->udpSocket);
```
Callers can then check for `-1` and handle the failure case explicitly.
```suggestion
int CoreThread::getUdpSock() const {
// TODO: should no longer use it
if (!pImpl->udpSocket) {
return -1;
}
return g_socket_get_fd(pImpl->udpSocket);
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #697 +/- ##
==========================================
- Coverage 52.37% 52.00% -0.37%
==========================================
Files 64 64
Lines 8281 8599 +318
==========================================
+ Hits 4337 4472 +135
- Misses 3944 4127 +183 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Replace listen/poll/accept in RecvTcpData with GIO equivalents (g_socket_listen, g_socket_condition_timed_wait, g_socket_accept) - Replace shutdown calls in ClearSublayer with g_socket_close - Update TcpData to accept GSocket* instead of raw fd - Replace getpeername calls with g_socket_get_remote_address - TcpData now owns and manages the GSocket lifecycle This improves portability for Windows by using GIO's cross-platform socket abstraction instead of Unix-specific APIs. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add TcpThread struct similar to UdpThread with GMainLoop/GMainContext - Use g_socket_create_source for event-driven TCP accept - Use g_main_context_invoke for clean thread shutdown instead of polling a flag in a loop - Remove RecvTcpData polling function, replaced by tcpThreadCb callback - Add CORE_THREAD_ERR_TCP_THREAD_START_FAILED error code - Simplify ClearSublayer (socket cleanup handled by destructors) This makes TCP thread handling consistent with UDP thread and removes the 10ms polling loop for better efficiency and cleaner shutdown. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Change TCP handler from detached std::thread to managed GThread - Track handler threads in std::list for O(1) removal - Clean up finished threads via callback on TCP thread's main context - Join all handler threads on CoreThread stop - Add exception handling in TCP handler thread - Add unit tests for TCP handler thread lifecycle Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Migrate UDP handling in CoreThread to use GLib/GIO UDP sockets and socket addresses instead of raw file descriptors and sockaddr structures.
New Features:
Bug Fixes:
Enhancements:
Build: