Modify log in udpserver.c #1314

KangLin · 2023-11-10T08:14:49Z

Modify log in udpserver.c
fix memory leak

KangLin · 2023-11-14T02:44:35Z

New output contents:

10:42:45: Debugging /home/build-coturn-Desktop_Qt_5_12_12_GCC_64bit-Debug/bin/turnutils_peer -v -L 192.168.11.30
...
0: (13865): INFO: 192.168.11.30:3480 start
0: (13865): INFO: 192.168.11.30:3481 start
7: (13865): DEBUG: Received 100 bytes data 192.168.11.30:3480 from 192.168.11.49:56473. total received: 100 bytes
7: (13865): DEBUG: Received 100 bytes data 192.168.11.30:3480 from 192.168.11.49:56473. total received: 200 bytes
7: (13865): DEBUG: Received 100 bytes data 192.168.11.30:3480 from 192.168.11.49:56473. total received: 300 bytes
7: (13865): DEBUG: Received 100 bytes data 192.168.11.30:3480 from 192.168.11.49:56473. total received: 400 bytes
7: (13865): DEBUG: Received 100 bytes data 192.168.11.30:3480 from 192.168.11.49:56473. total received: 500 bytes
10: (13865): DEBUG: Received 100 bytes data 192.168.11.30:3481 from 192.168.11.49:62882. total received: 100 bytes
10: (13865): DEBUG: Received 100 bytes data 192.168.11.30:3481 from 192.168.11.49:62882. total received: 200 bytes
10: (13865): DEBUG: Received 100 bytes data 192.168.11.30:3481 from 192.168.11.49:62882. total received: 300 bytes
10: (13865): DEBUG: Received 100 bytes data 192.168.11.30:3481 from 192.168.11.49:62882. total received: 400 bytes
10: (13865): DEBUG: Received 100 bytes data 192.168.11.30:3481 from 192.168.11.49:62882. total received: 500 bytes
10:43:02: Debugging of /home/build-coturn-Desktop_Qt_5_12_12_GCC_64bit-Debug/bin/turnutils_peer -v -L 192.168.11.30
has finished with exit code 0.

eakraly · 2023-11-19T05:59:28Z

src/apps/peer/udpserver.c

-  stun_buffer buffer;
-  ioa_addr remote_addr;
+  int slen = get_ioa_addr_len(&listen->addr);
+  stun_buffer *buffer = (stun_buffer *)malloc(sizeof(stun_buffer));


What is the reason to use heap instead of stack as before? It does not go ouside of the function and actually released at the end of it?

This size is large. then maybe stack overflow.

Default stack sizes are large - usually 1MB
Here we are talking about packet size - 1500B? Did you have an issue?
Not that it matters much for a test util but I would still not include unexplained changes to code

I agree with eakraly

the size of a stun packet shouldn't be large enough to justify always mallocing.

If you're concerned about stack size, you could try to support dynamic buffers, with a default buffer of 1500 on the stack, and then larger buffers allocated if necessary because the incoming data is large enough to justify it.

Though given the nature of async code, i'm not sure it would be possible to determine that prior to attempting to read.

To summarize, I have a fairly strong preference that this buffer be on the stack not on the heap, especially since I've spent the last week trying to track down memory leaks in coturn and C-language code is notorious for memory problems in general.

src/apps/peer/udpserver.c

KangLin · 2023-11-20T01:08:22Z

Default stack sizes are large - usually 1MB

Warning prompt during clang grammar checking. Some compilers are very small, only 516. Whether it's a testing program or not, a good habit should be maintained. At 2023-11-20 04:06:53, "Pavel Punsky" ***@***.***> wrote: @eakraly commented on this pull request. In src/apps/peer/udpserver.c:

int len = 0; - int slen = get_ioa_addr_len(addr); - stun_buffer buffer; - ioa_addr remote_addr; + int slen = get_ioa_addr_len(&listen->addr); + stun_buffer *buffer = (stun_buffer *)malloc(sizeof(stun_buffer)); Default stack sizes are large - usually 1MB Here we are talking about packet size - 1500B? Did you have an issue? Not that it matters much for a test util but I would still not include unexplained changes to code — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: ***@***.***>

- fix memory leak

KangLin · 2023-11-23T06:01:18Z

@eakraly pls merge!

jonesmz · 2024-01-19T20:48:28Z

src/apps/peer/mainudpserver.c

@@ -58,7 +58,7 @@ int main(int argc, char **argv) {
  char **local_addr_list = NULL;
  size_t las = 0;
  int verbose = TURN_VERBOSE_NONE;
-  int c;
+  int c = 0;


can int c be moved down in to the while loop instead ?

jonesmz · 2024-01-19T20:48:59Z

src/apps/peer/mainudpserver.c

@@ -98,6 +98,8 @@ int main(int argc, char **argv) {
  }

  server_type *server = start_udp_server(verbose, ifname, local_addr_list, las, port);
+  if (!server)
+    return -3;


can these return codes be made into named constants?

jonesmz · 2024-01-19T20:53:06Z

src/apps/peer/udpserver.c

+  int slen = get_ioa_addr_len(&listen->addr);
+  stun_buffer *buffer = (stun_buffer *)malloc(sizeof(stun_buffer));
+  ioa_addr remote_addr = {0};
+  if (!buffer)


Braces please

jonesmz · 2024-01-19T20:56:03Z

src/apps/peer/udpserver.c


  do {
-    len = recvfrom(fd, buffer.buf, sizeof(buffer.buf) - 1, 0, (struct sockaddr *)&remote_addr, (socklen_t *)&slen);
+    len = recvfrom(fd, buffer->buf, sizeof(buffer->buf) - 1, 0, (struct sockaddr *)&remote_addr, (socklen_t *)&slen);
  } while (len < 0 && socket_eintr());


This is attempting to read until recvfrom stops failing? recvfrom returns a positive number representing how many bytes were read, or a negative number on failure, and 0 on "no messages available".

I find the way this is setup to be really weird, but maybe that's just the design and i'm not familiar enough with the project to understand yet.

jonesmz · 2024-01-19T20:57:35Z

src/apps/peer/udpserver.c


  if (len >= 0) {
+    listen->received += len;
+    if (listen->verbose) {
+      uint8_t ra[64] = {0}, la[64] = {0};


magic numbers. Please use a named constant or some kind of sizeof() calculation.

Also please don't declare multiple variables on the same line, it makes it impossible to understand what's going on

jonesmz · 2024-01-19T20:59:16Z

src/apps/peer/udpserver.c

    do {
-      len = sendto(fd, buffer.buf, buffer.len, 0, (const struct sockaddr *)&remote_addr, (socklen_t)slen);
+      len = sendto(fd, buffer->buf, buffer->len, 0, (const struct sockaddr *)&remote_addr, (socklen_t)slen);


Maybe i'm not understanding the design properly, but why are we sending the buffer we just read...? Doesn't the buffer need to be checked for contents of any sort?

Unless this particular application is simply intended to spit back the exact buffer it received.
If so, that doesn't seem all that useful, but i could use it in specific test situations.

This app is an implementation of "peer" so that tests could be run without relying on external code/clients. Echoing back the received packets is what it does - for tests exactly @jonesmz

KangLin · 2024-01-20T06:15:07Z

It's just a simple echo. Statistics will be added later. At 2024-01-20 04:59:28, "Michael Jones" ***@***.***> wrote: @jonesmz commented on this pull request. In src/apps/peer/udpserver.c:

do {

- len = sendto(fd, buffer.buf, buffer.len, 0, (const struct sockaddr *)&remote_addr, (socklen_t)slen); + len = sendto(fd, buffer->buf, buffer->len, 0, (const struct sockaddr *)&remote_addr, (socklen_t)slen); Maybe i'm not understanding the design properly, but why are we sending the buffer we just read...? Doesn't the buffer need to be checked for contents of any sort? — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: ***@***.***>

KangLin force-pushed the udpserver branch 8 times, most recently from f9b4cdc to 7859523 Compare November 14, 2023 02:43

KangLin force-pushed the udpserver branch from 7859523 to 980c883 Compare November 14, 2023 02:47

eakraly reviewed Nov 19, 2023

View reviewed changes

- Modify log in udpserver.c

64a3080

- fix memory leak

KangLin force-pushed the udpserver branch from 980c883 to 64a3080 Compare November 20, 2023 01:13

KangLin mentioned this pull request Nov 23, 2023

Refactor: Modify udp_recvfrom. Raplace recvmsg with WSARecvMsg in windows #1324

Open

jonesmz reviewed Jan 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify log in udpserver.c #1314

Modify log in udpserver.c #1314

KangLin commented Nov 10, 2023 •

edited

KangLin commented Nov 14, 2023

eakraly Nov 19, 2023 •

edited

KangLin Nov 19, 2023

eakraly Nov 19, 2023

jonesmz Jan 19, 2024

jonesmz Jan 19, 2024

KangLin commented Nov 20, 2023 via email

KangLin commented Nov 23, 2023

jonesmz Jan 19, 2024

jonesmz Jan 19, 2024

jonesmz Jan 19, 2024

jonesmz Jan 19, 2024

jonesmz Jan 19, 2024

jonesmz Jan 19, 2024

jonesmz Jan 19, 2024

eakraly Jan 28, 2024

KangLin commented Jan 20, 2024 via email

Modify log in udpserver.c #1314

Are you sure you want to change the base?

Modify log in udpserver.c #1314

Conversation

KangLin commented Nov 10, 2023 • edited

KangLin commented Nov 14, 2023

eakraly Nov 19, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KangLin commented Nov 20, 2023 via email

KangLin commented Nov 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KangLin commented Jan 20, 2024 via email

KangLin commented Nov 10, 2023 •

edited

eakraly Nov 19, 2023 •

edited