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.

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

Proposal / discussion: Build all of coturn as C++17, instead of C11. #1416

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jonesmz
Copy link
Contributor

@jonesmz jonesmz commented Jan 29, 2024

Resolves #1389

Currently coturn is compiled with each source file treated as C-language files, specifically C11 (though, the adoption of features from C11 is not very far along, e.g. stdbool.h).

This pull request changes the compilation settings of both the Makefile based, and CMake based builds of coturn to instead treat the source files as C++17.

Note: In the linked issue I asked about patches to support compiling as C++ instead of actually compiling as C++. This PR was so easy to put together I figured I'd skip straight to feeling the waters on full adoption before bothering only submitting the minor changes needed support "Optionally, but not always, allow compiling as C++". If the idea of switching to C++ is outright rejected, I'll still want to put in the compile-as-C++ fixes, but would strongly prefer we just go for it.

Note: This specific PR does not rename the C files to have the .cpp extension, which would cause CMake to implicitly assume they are C++ files, because I did not want to make the diff crazy looking. If the maintainers of coturn are interested in adopting C++ instead of C-language, then it would be less messy to rename the existing files instead of instructing CMake to treat the files as C++.

Motivation

  • Almost all C code can be compiled as C++ code. Inherently there is no need for the coturn project to adopt C++ specific functionality that it does not want, but compiling as C++ is a prerequisite for being able to use any C++ features at all.

I'll put a break in the list here to emphasis this much more strongly.

I am NOT saying

"Lets re-write the project in C++ guys!!!!"

I AM saying

"Lets treat the C-language code as C++, and then adopt language features that make sense to adopt, incrementally".

  • I, and the rest of my group at my work, use C++ neigh exclusively, so I will be substantially more effective at making contributions to coturn when writing in a language that I am familiar with.
    • Now that I am the service-owner of my work's turnserver service (using coturn), I anticipate spending a substantial amount of time working with coturn, and... to be frank and perfectly honest, I really don't like C. This change will make me more effective and happier to help maintain the project.
  • Many, many, many of the problems with coturn, including the two most recent CVEs that I'm aware of, have to do with string handling, and the allocation, freeing, and pointer management thereof.
    • C++ has built in string handling facilities that are trivial to get right, and very difficult to screw up, specifically std::string, and various features related to it.
  • C++ is supported on almost every platform in existence.
    • If it has a C compiler, it probably has a C++ compiler, and that compiler is most likely GCC.
  • C++ has destructors, which act as a standardized form of the GCC extension __attribute__((cleanup)) (see https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html ).
  • C++ has standardized thread management, mutex management, and synchronization primitives built into the standard library natively supported on all platforms with a C++ compiler.
    • The use of pthreads can be replaced with a much more ergonomic threading system.
  • C++ has lambas, which are very thin syntax-sugar-wrappers around essentially a C-struct with a static function passed the struct as the first argument. (Or a C++ class with an operator() function, for those familiar with C++ terminology).
  • C++ has language-level virtual/interface classes, e.g. the equivalent of a struct of function-pointers, such as what is used in the database driver code, where each DB Driver populates a list of function pointers at startup and the main process calls into those function pointer to do whatever operation it's trying to do.
  • C++ has constexpr and inline variables and functions, which can help reduce the extern and #define soup that most of the coturn internal headers appear to be right now, while also providing the compiler better opportunities to optimize the resulting code.
  • Some tools, such as clang-tidy auto-fixits, just don't work with C-language, supporting only C++.


If nothing else, the C++ Destructors is the critical functionality that would improve the coturn project's security posture the most.

I would be happy to see coturn switch to C++ EVEN IF IT DOES NOT USE ANYTHING BUT C-language + C++Destructors.



I'll stop listing motivations there, even though I could probably write many pages of benefits.

Instead I'll write a bit more about what C++ will let me do in the short-term.

Followup after switching to C++

  • I am working on a health-check program to be used by the Amazon elastic load balancer system to query if the coturn process is in a healthy state, and shut the machine instance down (to be replaced with another) if it is not.
    • This program will be written in C++ regardless of the decision of this PR.
    • I also happen to be basing it off of the turnutils_uclient program, with the majority of functionality stripped out and some new behavior added. Switching coturn to C++ will let me submit a bunch of improvements to turnutils_uclient before forking it and removing the unnecessary bits (and adding the new bits)
  • I, or a coworker, can do a pass through the whole codebase and find locations where we have risky memory management, and apply C++ destructors to ensure resources are freed on function exit, to prevent memory leaks.
  • String management program wide could be massively simplified by replacing char* / char const* with std::string.
    • There might be a tiny growth in memory usage from this, but I'm skeptical it'll be much if any.
    • The reduction in cognitive complexity and memory-leak risks would be well worth it regardless of any increase in memory growth.
  • The DB drivers could be conceptually much improved by using a class with a bunch of pure-virtual functions (aka, an abstract base class, or interface class).
    • Doing this should make it much easier for clang-tidy scans to find usage problems.
  • The use of pthreads APIs could be essentially drop-in replaced with std::thread based functionality, improving platform compatibility, and allowing MSVC specific quirks to be removed entirely.

Those are all things that I have the flexibility to implement in the short-term and make PRs for. I don't have unlimited time, but I have several weeks where coturn is my primary focus.

FAQ

  • Am I crazy?
    • I've got around 15 years of C++ experience, and the first 5 of those were C-library code plus C++ GUI code, and it was an overwhelming nightmare. Since switching to full C++, I've found it substantially easier to write complex program in a small amount of time. You just cannot be as productive in C-language as you can be in C++, and the C-language code you wrote probably has several bugs that the equivalent C++ code wouldn't have.
  • Don't C++ programs break ABI / API of public headers all the time?
    • Not really, no. Compiling the source code as C++ doesn't mean that the public interface of the coturn libraries (are there any...? I see client, but i'm not sure what even uses that?) would have to change. The public interface can remain C-language code without interfering with the implementation files compilation language. This isn't really even more or less work, as the C-language public interface already exists, and any C++-isms can be exposed to the world as "also" instead of "instead of".
  • Isn't C++ slower than C?
    • No?
      • If you implement a function with C code, and compile that as C and also C++, the resulting machine code is more likely than not going to be identical.
    • C++ templates can be much faster than C code, but they can also quickly result in many different instantiations of the same function with different inputs. This can make the program much bigger, and that can cause slowdowns due to cache misses and similar considerations.
      • The easy way to avoid this problem is.... don't do that? I'm not proposing that coturn adopt C++templates. While there might be some place in the coturn code that could benefit from templates, I have not yet encountered it.
  • Isn't C++ full of foot-guns?
    • No more than C-language code is. Arguably, C-language has fewer foot-guns, but they have much easier to pull triggers. C++ has more simply due to having more language features, but they're harder to aim and fire at feet.
    • Nothing needs to be re-written in one go, the code that compiles as C++ in this PR should behave essentially identical to the code that compiles as C-language without this PR. It's only when changes are submitted that adopt C++ functionality that there's any change in the foot-guns.
  • Isn't C++ supported on fewer platforms than C-language?
    • If there's a platform that you care about, and it isn't part of the continuous integration, you don't really care about it.
    • I think the vast, vast, vast, majority of users of coturn are running on Linux, Windows, Mac, BSD, or Solaris, and out of those 5, probably only Linux has anywhere close to over %1 of the users.
    • I don't think coturn as a project should attempt to support platforms that are so esoteric that the GCC project doesn't have a build for it. Those users are perfectly capable of continuing to use the version of coturn that they're using now, and fixes could be applied to a long-term-service branch of this repo to keep those users on life-support if any of them ever come asking.

@jonesmz
Copy link
Contributor Author

jonesmz commented Jan 29, 2024

Note that this PR does have two commits that aren't really related to the C++ stuff.

The first is: #1415
The other will be made into a new PR after #1415 is merged.

@@ -81,8 +69,6 @@ void prom_set_finished_traffic(const char *realm, const char *user, unsigned lon
void prom_inc_allocation(SOCKET_TYPE type);
void prom_dec_allocation(SOCKET_TYPE type);

#endif /* TURN_NO_PROMETHEUS */

#ifdef __cplusplus
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the closing brace for extern "C" { was mismatched in this file.

@@ -567,7 +567,7 @@ int stun_is_challenge_response_str(const uint8_t *buf, size_t len, int *err_code
const uint8_t *value = stun_attr_get_value(sar);
if (value) {
size_t vlen = (size_t)stun_attr_get_len(sar);
vlen = min(vlen, STUN_MAX_REALM_SIZE);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

C++ is a tad bit more picky about function arguments to std::min, so casting to size_t (to match vlen) fixes the compile error without any change in behavior.

"LOG_LOCAL1", "LOG_LOCAL2", "LOG_LOCAL3", "LOG_LOCAL4", "LOG_LOCAL5",
"LOG_LOCAL6", "LOG_LOCAL7", "LOG_LPR", "LOG_MAIL", "LOG_NEWS",
"LOG_USER", "LOG_UUCP", "LOG_AUTHPRIV", "LOG_SYSLOG", 0};
static char const *const str_fac[] = {"LOG_AUTH", "LOG_CRON", "LOG_DAEMON", "LOG_KERN", "LOG_LOCAL0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

undefined behavior to assign char* variables with string literals.

In both C and also C++.

Just C-language will let you do it anyway, and C++ won't (without a compiler flag that looks scary).

@KangLin
Copy link
Contributor

KangLin commented Feb 2, 2024

  • I am support to use C++
  • Suggest C+11, (Will be support C++17 after 5 year).
  • If we use C++, we rewrite the code, not refactor it. I think it's easier to rewrite than to refactor.
  • With three to five people working together, it should be easy 😄
    @paullouisageneau Interested in joining?

@jonesmz
Copy link
Contributor Author

jonesmz commented Feb 2, 2024

I'm not going to use a 10 year old, or older, version of the language. That's completely out of the question.

I'm also not interested in writing a new implementation from scratch.

My objective is to keep the same architecture, and largely the same code. My main interest in using c++ is string handling (std::string, std::string_view), and Destructors / RAII for resource cleanup. Other C++-ification would be fine, but isn't my main concern at this time.

@KangLin
Copy link
Contributor

KangLin commented Feb 2, 2024

I'm not going to use a 10 year old, or older, version of the language. That's completely out of the question.

I'm also not interested in writing a new implementation from scratch.

My objective is to keep the same architecture, and largely the same code. My main interest in using c++ is string handling (std::string, std::string_view), and Destructors / RAII for resource cleanup. Other C++-ification would be fine, but isn't my main concern at this time.

Then I think it's better to keep C!It is not advisable to mix C++ (std::string, std::string_view) in C.

@jonesmz
Copy link
Contributor Author

jonesmz commented Feb 2, 2024

Can you provide some explanation of why you think it is not advisable?

@Neustradamus
Copy link

To follow

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Will PRs to support compiling coturn as C++ code instead of C code be accepted?
3 participants