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

Remove the VLA from handle_connection() #109

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

Conversation

mixi
Copy link

@mixi mixi commented Apr 3, 2021

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 fixes #108.

@mixi
Copy link
Author

mixi commented Apr 3, 2021

I also now implemented the memory savings on glibc by requesting a reasonably small thread stack as proposed in #108.

@dashezup
Copy link

dashezup commented Apr 3, 2021

it only receives 8 bytes on f3c7a29 (tested on musl), no matter if -B is set or not.

============================================================
[Fiche][STATUS] Sat Apr  3 10:09:30 2021
[Fiche][STATUS] Incoming connection from: 127.0.0.1 (localhost.localdomain).
[Fiche][STATUS] Received 8 bytes, saved to: hmxd.
============================================================

or it's still wip? @ me when you need me to test it.

mixi added 2 commits April 3, 2021 12:25
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.
Returning from the start function of a thread is specified to
implicitly call pthread_exit().
@mixi mixi force-pushed the musl-thread-fix branch from f3c7a29 to fcfcff9 Compare April 3, 2021 10:25
@mixi
Copy link
Author

mixi commented Apr 3, 2021

@dashezup: That was me missing that sizeof(buffer) was being used to determine the size of the buffer (works with a static buffer / a VLA, doesn't work with a dynamically allocated buffer). It should be fixed now.

@dashezup
Copy link

dashezup commented Apr 3, 2021

Nice 👍 , I've tested fcfcff9 on musl, it works properly, I can upload very huge text (like 1G, probably memory was full and can't add more in that case)

// Cleanup
free(c);
free(slug);
close(c->socket);
Copy link
Author

Choose a reason for hiding this comment

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

NOTE: This was broken before moving the cleanup to the exit label because it called free(c) before dereferencing c.

This somewhat mitigates the problem that now the buffer is allocated in
addition to the already allocated thread stack.
@mixi mixi force-pushed the musl-thread-fix branch from fcfcff9 to 0771a63 Compare April 3, 2021 10:52
@mixi
Copy link
Author

mixi commented Apr 3, 2021

That was me changing the 16k of requested thread stack to the musl default of 128k. That should probably be safer and still less than the glibc default.

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.

Segmentation fault on musl when set big buffer size
2 participants