Skip to content

Explicitly use void in the C function list of arguments for functions that take no arguments#38307

Merged
adisuissa merged 2 commits into
envoyproxy:mainfrom
krinkinmu:fix-ubsan-on-clang-18
Feb 5, 2025
Merged

Explicitly use void in the C function list of arguments for functions that take no arguments#38307
adisuissa merged 2 commits into
envoyproxy:mainfrom
krinkinmu:fix-ubsan-on-clang-18

Conversation

@krinkinmu
Copy link
Copy Markdown
Contributor

@krinkinmu krinkinmu commented Feb 4, 2025

Commit Message:

Using void in the list of function arguments in C++ is allowed for functions that take no arguments, but it's rather uncommon.

However, when it comes to plain C, there is an actual difference between a function declaration int foo() and int foo(void). The first one says that there is a function foo that returns an integer and nothing is known about the arguments the function takes. While the second one explicitly declares that the function takes no argument.

In practice this difference does not often matter, however, from the language point of view, they are not the same and it triggers UBSAN when running Envoy tests on clang-18.

Specifically, UBSAN complains that tests attempt to call a function thought a pointer of incompatible function type.

I don't believe it causes any issues in practice for Envoy, but addressing these warnings will help us to move to a newer version of LLVM toolchains for Envoy.

NOTE: There are some alternative ways we could address this issue as well, for example, we can change all the examples to use C++ instead of C, and, I belive, it would also address the issue, but it's a bit more invasive and I opted not to convert all the tests because of is practically a false positive UBSAN complain.

Additional Description:

Related to #37911 and fixes one of the issues in #38093

This is the best write-up on the topic of using void in the paramters list I could find to elaborate: https://wiki.sei.cmu.edu/confluence/display/c/DCL20-C.+Explicitly+specify+void+when+a+function+accepts+no+arguments#:~:text=Defining%20a%20function%20with%20a,%5BTIGCC%2C%20void%20usage%5D..

Risk Level: low
Testing: bazel test -c dbg //test/extensions/dynamic_modules:dynamic_modules_test --config=docker-asan
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a

+cc @phlax

that take no arguments

Using void in the list of function arguments in C++ is allowed for
functions that take no arguments, but it's rather uncommon.

However, when it comes to plain C, there is an actual difference between
a function declaration `int foo()` and `int foo(void)`. The first one
says that there is a function foo that returns an integer and nothing is
known about the arguments the function takes. While the second one
explicitly declares that the function takes no argument.

In practice this difference does not often matter, however, from the
language point of view, they are not the same and it triggers UBSAN when
running Envoy tests on clang-18.

Specifically, UBSAN complains that tests attempt to call a function
thought a pointer of incompatible function type.

I don't believe it causes any issues in practice for Envoy, but
addressing these warnings will help us to move to a newer version of
LLVM toolchains for Envoy.

NOTE: There are some alternative ways we could address this issue as
well, for example, we can change all the examples to use C++ instead of
C, and, I belive, it would also address the issue, but it's a bit more
invasive and I opted not to convert all the tests because of is
practically a false positive UBSAN complain.

Signed-off-by: Mikhail Krinkin <mkrinkin@microsoft.com>
@repokitteh-read-only
Copy link
Copy Markdown

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #38307 was opened by krinkinmu.

see: more, trace.

Signed-off-by: Mikhail Krinkin <mkrinkin@microsoft.com>
@krinkinmu krinkinmu marked this pull request as ready for review February 4, 2025 18:26
@adisuissa
Copy link
Copy Markdown
Contributor

Assigning @mathetake as extension code-owner.
/assign @mathetake

Copy link
Copy Markdown
Member

@mathetake mathetake left a comment

Choose a reason for hiding this comment

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

Thank you!

Copy link
Copy Markdown
Contributor

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@adisuissa adisuissa merged commit 4eeb403 into envoyproxy:main Feb 5, 2025
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.

3 participants