Skip to content

Fix #678: Fix stats print on ARM due to indeterminate state of "va_list" parameter after vasprintf #933

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.

Sign up for GitHub

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

Closed
IgnoreWarnings wants to merge 2 commits into from
Closed

Fix #678: Fix stats print on ARM due to indeterminate state of "va_list" parameter after vasprintf #933

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fbb4555
fix: Portability of vstrcatf and vstrf
IgnoreWarnings Jun 26, 2025
844a1c1
fix: vstrcatf wil leave va_list parameter valid after use
IgnoreWarnings Jun 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions common/include/villas/utils.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,11 +163,11 @@ char *strcatf(char **dest, const char *fmt, ...)
__attribute__((format(printf, 2, 3)));

// Variadic version of strcatf()
char *vstrcatf(char **dest, const char *fmt, va_list va)
char *vstrcatf(char **dest, const char *fmt, va_list &va)
__attribute__((format(printf, 2, 0)));

char *strf(const char *fmt, ...);
char *vstrf(const char *fmt, va_list va);
char *vstrf(const char *fmt, va_list &va);

// Allocate and copy memory.
void *memdup(const void *src, size_t bytes);
Expand Down
20 changes: 17 additions & 3 deletions common/lib/utils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,10 +194,24 @@ double boxMuller(float m, float s) {

double randf() { return (double)random() / RAND_MAX; }

char *vstrcatf(char **dest, const char *fmt, va_list ap) {
char *vstrcatf(char **dest, const char *fmt, va_list &ap) {
Copy link
Copy Markdown
Member

@n-eiling n-eiling Jun 26, 2025

Choose a reason for hiding this comment

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

I'm surprised you don't need const char* restrict fmt.

Copy link
Copy Markdown
Contributor

@stv0g stv0g Jun 27, 2025

Choose a reason for hiding this comment

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

To my knowledge restrict is a C not C++ keyword?

Are you concerned about aliasing between the *dest and fmt pointers?

I dont fully understand, why we might need it..

char *tmp;
Copy link
Copy Markdown
Member

@n-eiling n-eiling Jun 26, 2025

Choose a reason for hiding this comment

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

I would add null pointer checks for dest and fmt

int n = *dest ? strlen(*dest) : 0;
int i = vasprintf(&tmp, fmt, ap);

va_list va_copy;
va_copy(va_copy, ap);
int i = vasprintf(&tmp, fmt, va_copy);
va_end(va_copy);

// Advance va_list
const char *format = fmt;
if (strcmp(format, "%ju") == 0) {
va_arg(ap, unsigned int);
} else if (strcmp(format, "%lf") == 0) {
va_arg(ap, double);
} else if (strcmp(format, "%s") == 0) {
va_arg(ap, char *);
}
Copy link
Copy Markdown
Member

@n-eiling n-eiling Jun 26, 2025

Choose a reason for hiding this comment

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

Not sure if this is real-time critical code. But you could avoid one allocation by doing:

size_t n = vsnprintf(NULL, 0, fmt, va_copy);
*dest = (char *)(realloc(*dest, n + i + 1));
vsnprintf(*dest + n, n+1, fmt, va_copy);

Copy link
Copy Markdown
Contributor

@stv0g stv0g Jun 27, 2025

Choose a reason for hiding this comment

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

I am wondering, isnt it also UB to call vsnprintf again with the same va_copy?

@IgnoreWarnings wrote in #678

This bug occures because "va_list" is used after beeing passed to "vasprintf", which leads to undeterministic behavior.
Functions like "vasprintf" "invoke va_arg at least once, the value of arg is indeterminate after the return." (https://en.cppreference.com/w/c/io/vfprintf : Notes).


*dest = (char *)(realloc(*dest, n + i + 1));
if (*dest != nullptr)
Expand Down Expand Up @@ -228,7 +242,7 @@ char *strf(const char *fmt, ...) {
return buf;
}

char *vstrf(const char *fmt, va_list va) {
char *vstrf(const char *fmt, va_list &va) {
char *buf = nullptr;

vstrcatf(&buf, fmt, va);
Expand Down