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

Fix crash on FreeBSD due to incorrect thr_self system call invocation #285

Merged
merged 1 commit into from
Aug 16, 2024

Conversation

yurivict
Copy link
Contributor

The correct signature is:
int thr_self(long *id);

It was called as thr_self() which caused memory corruption.


Alternatively, it can be called directly, without syscall, according to its manpage:

NAME
     thr_self – return thread identifier for the calling thread

LIBRARY
     Standard C Library (libc, -lc)

SYNOPSIS
     #include <sys/thr.h>

     int
     thr_self(long *id);

Other *BSDs likely have the same problem.

The correct signature is:
int thr_self(long *id);

It was called as thr_self() which caused memory corruption.
@yurivict
Copy link
Contributor Author

Additionally, this patch also wouldn't hurt:

--- src/libs/logging.c.orig     2024-08-15 18:51:51 UTC
+++ src/libs/logging.c
@@ -31,4 +31,4 @@ bool us_g_log_colored;

 bool us_g_log_colored;

-pthread_mutex_t us_g_log_mutex;
+pthread_mutex_t us_g_log_mutex = PTHREAD_MUTEX_INITIALIZER;

I am not sure if PTHREAD_MUTEX_INITIALIZER is required, but PTHREAD_MUTEX_INITIALIZER is defined so it might make sense to use it.

@mdevaev
Copy link
Member

mdevaev commented Aug 16, 2024

Thanks. I have superficially checked other BSDs, it should be ok. In any case, I will be waiting for bug reports.

About the mutex - it's initialized inside US_LOGGING_INIT().

@mdevaev mdevaev self-assigned this Aug 16, 2024
@mdevaev mdevaev added the type:bug Something isn't working label Aug 16, 2024
@mdevaev mdevaev merged commit dcddfdd into pikvm:master Aug 16, 2024
1 of 2 checks passed
@mdevaev
Copy link
Member

mdevaev commented Aug 16, 2024

6.13 released with this fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
resolution:fixed Fixed type:bug Something isn't working
Development

Successfully merging this pull request may close these issues.

2 participants