Marking variables as const when they won't be modified after
initialization helps programmers trying to understand a codebase to
manage the cognative load.
This pull request uses a clang-tidy fixit (Hard to automate, since the
code needs to be temporarily compiled as C++ for it to work) to try to
mechanically apply the const keyword to code where the automated tool
can determine that the variable won't be modified.
I then follow this up with a manual improvement pass to
turnutils_uclient, where I address const correctness of local variables,
as well as do some adjustments to loops and scoping to help with
reducing complexity.
Co-authored-by: redraincatching <redraincatching@disroot.org>
Co-authored-by: Pavel Punsky <eakraly@users.noreply.github.com>
TLSv1 and TLSv1.1 can be enabled using `--tlsv1` and `--tlsv1_1`
arguments accordingly
That assumes openssl version being used has these versions enabled
(which as of openssl-3.5 is not by default)
With notable exceptions of:
src/apps/common/win/*
src/apps/relay/telnet.*
The purpose of this change is to add the SPDX tags from
https://spdx.dev/, which is a linux foundation project, to the source
code.
This provides automated code provenance tools, which are used in setting
up software bill of materials reports, an easy time verifying that the
code license is known and no incompatibilities are present in a
codebase.
No copyright date, author, or license changes are made.
Note also that
7e525c8e1c
is the original commit for the ACME code (acme.h and acme.c) which was
then moved to acme.h and acme.c in this commit
d4686750ee
but neither commit indicates what license the ACME code was submitted
as.
https://github.com/coturn/coturn?tab=License-1-ov-file#readme is the
3-clause BSD license, but https://github.com/coturn/coturn/pull/672
documents that the author's intent was for the MIT license. So I've used
the SPDX tag and content of the MIT license for this change.
This adjusts the code to allow compilation with a C++ compiler, but
doesn't change the build to use a C++ compiler. Everything should
continue working as-is with existing c-compilers. This is just a "let it
work" change, not a "change how it works" change.
With requiring openssl version at least 1.1.1 all versions of TLS (up to
and including 1.3) and DTLS 1.2 are supported
With that, no detection or ability to disable a version will be provided
Openssl 1.1.1 is end-of-life in September 2023.
This PR removes support for versions of openssl OLDER than 1.1.1
1.1.1 should still be usable after this change is merged.
I don't see any value in supporting 1.1.1, but didn't see a reason to
purge support for 1.1.1 when there are so few checks for >= 3.0.
Note that this does also remove CI support for Ubuntu 16.04. The
official version of OpenSSL from Ubuntu for this release is listed here:
https://launchpad.net/ubuntu/+source/openssl as 1.0.2g
Since no newer releases of coturn will be backported by Canonical to
Ubuntu 16.04, anyone using Coturn on this operating system will have to
download and compile it themselves. They may build their own version of
OpenSSL if they somehow cannot upgrade to a newer version of Ubuntu.
My position is that these users should prefer to upgrade to a newer
operating system than worry about chasing newer releases of Coturn.
Co-authored-by: Pavel Punsky <eakraly@users.noreply.github.com>
The point of this change is to make the build instructions a bit more
straight forward. Since the hiresevent2 source files are only ever used
by the relay target, this scoping makes sense in general.
Use the include-what-you-use program to (partially) clean up header
includes, so that only includes which are needed, and no includes that
are not needed (or at least closer to that ideal) are done.
For a c-language project, the build-time improvements from this change
is minimal. This would have a much bigger impact on a C++ project than a
C-project for build times.
So for coturn, this change is mostly intended to just provide
consistency and make it easier to locate weird issues like strange
dependencies, and unnecessary connections between code.
Converts all of the variables in the uclient program that should be bool
but weren't.
A few other minor adjustments made at the behest of clang-tidy, but this
change does not address all of clang-tidy's complaints.
Fixed an issue in libevent's CMake configuration where pthreads were not
correctly added to the optional components list, leading to a
compilation error. #1448
Co-authored-by: linwenchen <wenchen0803@qq.com>
- Why? Because code where conditionals lack braces is much harder to read, and prone to indentation confusion.
- How? Just added an extra flag to .clang-format and re-ran clang-format on all the files.
I also moved .clang-format up to the top level of the repo so that it can be applied to the fuzz targets as well.
- srandom/random provide stronger randomness characteristics than
srand/rand in some operating systems.
- usage of srand/rand is not very consistent in coturn.
There is room for more refactoring and use apputils helper functions in
ns_turn_msg.c too but i'm not sure that dependency from "client" module
to "apps" module is a good idea yet.
Thx @0xdea
Co-authored-by: Gustavo Garcia <gustavogb@mail.com>
Add missing checks for length of realm/nonce/server_name before copying
those values to the buffer passed to stun_is_challenge_response_str.
The function stun_is_challenge_response_str is only used in uclient test
application.
Thank you very much @0xdea
Co-authored-by: Gustavo Garcia <gustavogb@mail.com>
Environment:
- Windows 10
- Cygwin 2.925
make output:
```
<command-line>: note: this is the location of the previous definition
src/apps/common/ns_turn_utils.c:53:10: fatal error: sys/syscall.h: No such file or directory
53 | #include <sys/syscall.h>
| ^~~~~~~~~~~~~~~
compilation terminated.
In file included from src/client/ns_turn_ioaddr.h:34,
from src/client/ns_turn_msg.h:34,
from src/apps/common/stun_buffer.h:34,
from src/apps/common/stun_buffer.c:31:
src/ns_turn_defs.h:223: warning: "TURN_NO_SCTP" redefined
223 | #define TURN_NO_SCTP
|
<command-line>: note: this is the location of the previous definition
make: *** [Makefile:127: bin/turnutils_oauth] Error 1
```
PR #855 introduced new include <ssys/sysinfo.h>
It is not required for compilation or turnserver function but breaks
OpenBSD build (which does not have this file)
This PR removes the include to restore OpenBSD build compatibility
Fixes#1162
Test Plan:
TBD - need some one to test build
Reformatting and removing some duplications:
- Some lines have WARNING WARNING: cleaned up.
- Lines printed using perror: only LOG_ mechanism should be used.
- Printing IO mechanism (epoll for example) for each thread: selected
mechanism logged once
- Duplicate lines (perror and also LOG): duplication removed
- Duplicates: clean up (because calling function multiple times -
configuration load)
I would like to get feedback on this and see if people is confortable
with these clang rules.
Right now is using the "llvm" style increasing the line length from 80
to 120 given that coturn is using long lines often.
Co-authored-by: Pavel Punsky <eakraly@users.noreply.github.com>
`TURN_NO_HIREDIS` is defined when hiredis library is not present and any
redis functionality must be disabled
While all above is correct, it does not require ifdef-ing out all
related code.
For example, redis related fields in `turn_params` do not need to be
compiled out. Same for certain function parameters.
This PR reduces amount of places in code where `TURN_NO_HIREDIS` is used
to make code simpler by moving as much usage of this define into
dbd_redis.h/c files and compiling them unconditionally.
- Always compile/link `dbd_redis.c`
- Move many `TURN_NO_HIREDIS` decisions into `dbd_redis.c`
- Delete empty function redis_async_init
The following changes have been made:
1. Replace deprecated functions with new standard functions
2. Add corresponding MSVC functions for non-standard functions
3. Remove warnings about unsafe functions
4. CMAKE: modify find pack Libevent and openssl
5. Modify include files
6. Use pthread4W
7. Modify socket in windows
8. Add CI - github action
8.1. msvc
8.2. mingw
10. The database:
9.1. sqlite, pgsql, hiredis, mongo is test compiled.
9.2. mysql, isnot test compiled.
11. The applications、server can be compiled and run successfully!
12. Add vcpkg manifest mode in cmake.
Hello,
while using the `redis-statsdb` option, I found that coturn is leaking
sockets (and memory) in case of redis reconnection.
This occurs a lot to me, because in my setup I have a `coturn -> haproxy
-> redis` and if all my redis servers are down, HaProxy abruptly close
the connection to coturn and coturn reconnects periodically. After some
time I can see thousands of pending sockets (`CLOSE_WAIT`) :
```
user@server[11:32:48 UTC]:~$ sudo lsof -i | grep turn
turnserve 461797 root 15u IPv4 12856075 0t0 TCP server:3478 (LISTEN)
turnserve 461797 root 22u IPv4 12856081 0t0 TCP server:3478 (LISTEN)
turnserve 461797 root 23u IPv4 12857384 0t0 UDP server:3478
turnserve 461797 root 24u IPv4 12857385 0t0 UDP server:3478
turnserve 461797 root 36u IPv4 12857390 0t0 TCP server:5766 (LISTEN)
turnserve 461797 root 43u IPv4 12856096 0t0 TCP server:10059->haproxy-server:redis (CLOSE_WAIT)
turnserve 461797 root 46u IPv4 12857403 0t0 TCP server:10087->haproxy-server:redis (CLOSE_WAIT)
turnserve 461797 root 48u IPv4 12856124 0t0 TCP server:53867->haproxy-server:redis (CLOSE_WAIT)
turnserve 461797 root 50u IPv4 12857633 0t0 TCP server:53875->haproxy-server:redis (CLOSE_WAIT)
turnserve 461797 root 51u IPv4 12856138 0t0 TCP server:53877->haproxy-server:redis (CLOSE_WAIT)
turnserve 461797 root 54u IPv4 12857738 0t0 TCP server:10001->haproxy-server:redis (CLOSE_WAIT)
turnserve 461797 root 55u IPv4 12856152 0t0 TCP server:10003->haproxy-server:redis (CLOSE_WAIT)
.... (many many lines)
```
After searching and using valgrind I found 2 interesting leaks:
```
...
==460721== 32 bytes in 1 blocks are definitely lost in loss record 586 of 1,053
==460721== at 0x483877F: malloc (vg_replace_malloc.c:307)
==460721== by 0x1414FF: RyconninfoParse (dbd_redis.c:69)
==460721== by 0x141B04: get_redis_async_connection (dbd_redis.c:169)
==460721== by 0x110D7B: create_ioa_engine (ns_ioalib_engine_impl.c:407)
==460721== by 0x12ECB0: setup_admin_thread (turn_admin_server.c:1309)
==460721== by 0x127724: run_admin_server_thread (netengine.c:1815)
==460721== by 0x4DA9EA6: start_thread (pthread_create.c:477)
==460721== by 0x4EC0AEE: clone (clone.S:95)
...
==460979== 2,170 (688 direct, 1,482 indirect) bytes in 2 blocks are definitely lost in loss record 1,029 of 1,049
==460979== at 0x483AD7B: realloc (vg_replace_malloc.c:834)
==460979== by 0x49A1BD0: ??? (in /usr/lib/x86_64-linux-gnu/libhiredis.so.0.14)
==460979== by 0x49A2829: redisAsyncConnect (in /usr/lib/x86_64-linux-gnu/libhiredis.so.0.14)
==460979== by 0x13DB37: redis_reconnect (hiredis_libevent2.c:331)
==460979== by 0x13D1A7: redisLibeventReadEvent (hiredis_libevent2.c:101)
==460979== by 0x4D5135E: ??? (in /usr/lib/x86_64-linux-gnu/libevent_core-2.1.so.7.0.1)
==460979== by 0x4D51A9E: event_base_loop (in /usr/lib/x86_64-linux-gnu/libevent_core-2.1.so.7.0.1)
==460979== by 0x126D5A: run_events (netengine.c:1579)
==460979== by 0x127272: run_general_relay_thread (netengine.c:1707)
==460979== by 0x4DA9EA6: start_thread (pthread_create.c:477)
==460979== by 0x4EC0AEE: clone (clone.S:95)
==460979==
...
```
I made 1 commit for each fix.
Obviously with these fixes, I don't have anymore the leaks of thousands
of sockets (even after some time)
Thanks & hope it helps.
Thibaut
Using clang-tidy to detect unused header files
Inspired by #855
Test Plan:
- Rebuild all on mac, review no warnings/errors
- Pass builds/docker build - review for no issues
STRCPY macro makes pointer comparison which creates a warning
In those places, replace the macro with `strncpy` with careful review of
destination buffer size
With this change I do not get compiler warnings at all
Replace all instances of `bzero` with memset by find-replace-edit.
This is straightforward replacement which is suboptimal in a few cases
(for example we could use calloc instead of malloc+memset(0))
Inspired by #855
`vsnprintf` will stop at the max buffer size as provided in its 2nd
argument
But the return value is `The number of characters that would have been
written if n had been sufficiently large` meaning it can be larger than
actual buffer size
`fwrite` will actually use the larger, incorrect number and dump
unrelated memory to log (and crash with high confidence)
Test:
- Query admin interface with super long path (>16KB) - crash
- With the fix - no crash with the same input, log line cut off
Co-authored-by: Pavel Punsky <pavel.punsky@epicgames.com>
