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.

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

Jump to bottom

Segmentation fault on musl when set big buffer size #108

Open
dashezup opened this issue Apr 3, 2021 · 3 comments · May be fixed by #109
Open

Segmentation fault on musl when set big buffer size #108

dashezup opened this issue Apr 3, 2021 · 3 comments · May be fixed by #109

Comments

@dashezup
Copy link

dashezup commented Apr 3, 2021 •
edited
Loading

fiche -B 262144 gives me Segmentation fault while receiving the first upload on Void Linux musl, it works fine on Void Linux glibc so this issue is probably musl only.

[Fiche][STATUS] Starting fiche on Sat Apr  3 06:35:22 2021...
[Fiche][STATUS] Domain set to: http://example.com.
[Fiche][STATUS] Server started listening on port: 9999.
============================================================
[Fiche][STATUS] Sat Apr  3 06:35:27 2021
[Fiche][STATUS] Incoming connection from: 127.0.0.1 (localhost.localdomain).
[1]    18061 segmentation fault  fiche -B 262144

gdb debug log

Thread 2 "fiche" received signal SIGSEGV, Segmentation fault.
[Switching to LWP 23997]
0x00005555555561f5 in handle_connection (args=0x7ffff7ffe320) at fiche.c:565
565         memset(buffer, 0, c->settings->buffer_len);

fiche/fiche.c

Line 565 in 4bba916

memset(buffer, 0, c->settings->buffer_len);

@mixi
Copy link

mixi commented Apr 3, 2021

The problem is obvious: buffer is a VLA of the size c->settings->buffer_len in a non-main thread:

fiche/fiche.c

Line 524 in 4bba916

if ( pthread_create(&id, NULL, &handle_connection, c) != 0 ) {

fiche/fiche.c

Line 564 in 4bba916

uint8_t buffer[c->settings->buffer_len];

As such the code overflows the stack. The difference between musl and glibc for this test is that the default thread stack size is smaller on musl: https://wiki.musl-libc.org/functional-differences-from-glibc.html#Thread-stack-size
You can probably reproduce this problem on glibc with a far larger buffer size as well.

The solution should be to get rid of the VLA and instead to dynamically allocate the memory.

@dashezup
Copy link
Author

dashezup commented Apr 3, 2021 •
edited
Loading

@mixi Thanks. this explains why I get segmentation fault when the buffer size is greater than 128K (or less when I use -l log.txt).

musl provides a default stack size of 128k (80k prior to 1.1.21).

So the dirty fix for me would be -Wl,-z,stack-size=N at compile link time, I tried it and it works. Is there any disadvantage of using this? (such like use much more memory when there are a lot of uploads at the same time?)

The solution should be to get rid of the VLA and instead to dynamically allocate the memory.

I wonder, will that also benefit glibc users that reduces memory usage? I don't have C knowledge, maybe you or someone else else could provide a proper fix like this?

--
Just tested, it crashs on glibc too when the buffer size is set approach 8M.

mixi added a commit to mixi/fiche that referenced this issue Apr 3, 2021
@mixi
Remove the VLA from handle_connection()
f4170e4 
This fixes a segfault on musl libc with reasonable sized buffers, as
musl's default thread stack size is quite small (128k since 1.1.21).

A similar bug exists on glibc with large enough buffers (reproducable
with e.g. 16MB on my test system).

This commit fixes solusipse#108.
@mixi mixi linked a pull request Apr 3, 2021 that will close this issue
Open
@mixi
Copy link

mixi commented Apr 3, 2021 •
edited
Loading

So the dirty fix for me would be -Wl,-z,stack-size=N at compile link time, I tried it and it works. Is there any disadvantage of using this? (such like use much more memory when there are a lot of uploads at the same time?)

The disadvantage is that you are only moving the problem to a different point. It will still crash if you pass -B N+1 to it.

I wonder, will that also benefit glibc users that reduces memory usage? I don't have C knowledge, maybe you or someone else else could provide a proper fix like this?

The same problem exists for glibc, just with a larger buffer size. I was able to reproduce the same error there with 16MB of buffer.

My fix also does not save memory (it actually uses more memory, especially on glibc where it now needs to allocate the buffer in addition to their huge thread stack), but that could be mitigated by using pthread_attr_setstacksize() on glibc to request it to only use a reasonably small stack.

mixi added a commit to mixi/fiche that referenced this issue Apr 3, 2021
@mixi
Remove the VLA from handle_connection()
d428ccf 
This fixes a segfault on musl libc with reasonable sized buffers, as
musl's default thread stack size is quite small (128k since 1.1.21).

A similar bug exists on glibc with large enough buffers (reproducable
with e.g. 16MB on my test system).

This commit fixes solusipse#108.
drscream added a commit to drscream/fiche that referenced this issue Oct 19, 2021
@drscream @mixi
musl thread fix (#1)
847433d 
* Deduplicate the cleanup in handle_connection()

* Remove the VLA from handle_connection()

This fixes a segfault on musl libc with reasonable sized buffers, as
musl's default thread stack size is quite small (128k since 1.1.21).

A similar bug exists on glibc with large enough buffers (reproducable
with e.g. 16MB on my test system).

This commit fixes solusipse#108.

* Remove the superfluous call to pthread_exit

Returning from the start function of a thread is specified to
implicitly call pthread_exit().

* Request a reasonably small thread stack

This somewhat mitigates the problem that now the buffer is allocated in
addition to the already allocated thread stack.

Co-authored-by: Johannes Nixdorf <[email protected]>
drscream added a commit to drscream/fiche that referenced this issue Oct 19, 2021
@drscream @mixi
musl thread fix (#1)
58a8f12 
* Deduplicate the cleanup in handle_connection()

* Remove the VLA from handle_connection()

This fixes a segfault on musl libc with reasonable sized buffers, as
musl's default thread stack size is quite small (128k since 1.1.21).

A similar bug exists on glibc with large enough buffers (reproducable
with e.g. 16MB on my test system).

This commit fixes solusipse#108.

* Remove the superfluous call to pthread_exit

Returning from the start function of a thread is specified to
implicitly call pthread_exit().

* Request a reasonably small thread stack

This somewhat mitigates the problem that now the buffer is allocated in
addition to the already allocated thread stack.

Co-authored-by: Johannes Nixdorf <[email protected]>
drscream added a commit to drscream/fiche that referenced this issue Oct 19, 2021
@drscream @mixi
musl thread fix (#1)
036676a 
* Deduplicate the cleanup in handle_connection()

* Remove the VLA from handle_connection()

This fixes a segfault on musl libc with reasonable sized buffers, as
musl's default thread stack size is quite small (128k since 1.1.21).

A similar bug exists on glibc with large enough buffers (reproducable
with e.g. 16MB on my test system).

This commit fixes solusipse#108.

* Remove the superfluous call to pthread_exit

Returning from the start function of a thread is specified to
implicitly call pthread_exit().

* Request a reasonably small thread stack

This somewhat mitigates the problem that now the buffer is allocated in
addition to the already allocated thread stack.

Co-authored-by: Johannes Nixdorf <[email protected]>
drscream added a commit to drscream/fiche that referenced this issue Oct 19, 2021
@drscream @mixi
musl thread fix (#1)
1331508 
* Deduplicate the cleanup in handle_connection()

* Remove the VLA from handle_connection()

This fixes a segfault on musl libc with reasonable sized buffers, as
musl's default thread stack size is quite small (128k since 1.1.21).

A similar bug exists on glibc with large enough buffers (reproducable
with e.g. 16MB on my test system).

This commit fixes solusipse#108.

* Remove the superfluous call to pthread_exit

Returning from the start function of a thread is specified to
implicitly call pthread_exit().

* Request a reasonably small thread stack

This somewhat mitigates the problem that now the buffer is allocated in
addition to the already allocated thread stack.

Co-authored-by: Johannes Nixdorf <[email protected]>
borrougagnou pushed a commit to borrougagnou/maintenance-termbin that referenced this issue Nov 29, 2023
@mixi
Remove the VLA from handle_connection()
00885a5 
This fixes a segfault on musl libc with reasonable sized buffers, as
musl's default thread stack size is quite small (128k since 1.1.21).

A similar bug exists on glibc with large enough buffers (reproducable
with e.g. 16MB on my test system).

This commit fixes solusipse#108.
borrougagnou pushed a commit to borrougagnou/maintenance-termbin that referenced this issue Nov 29, 2023
@mixi
Remove the VLA from handle_connection()
a98f0fb 
This fixes a segfault on musl libc with reasonable sized buffers, as
musl's default thread stack size is quite small (128k since 1.1.21).

A similar bug exists on glibc with large enough buffers (reproducable
with e.g. 16MB on my test system).

This commit fixes solusipse#108.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Remove the VLA from handle_connection() mixi/fiche
2 participants
@mixi @dashezup