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

Use FD_CLOEXEC fcntl for WEB-server socket #248

Merged
merged 2 commits into from
May 22, 2016
Merged

Conversation

sysoleg
Copy link
Contributor

@sysoleg sysoleg commented Mar 4, 2016

free() is not safe in fork()ed child when using threads. Don't close
WEB-server socket manually, use FD_CLOEXEC fcntl instead.

See rationale here: http://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_atfork.html

free() is not safe in fork()ed child when using threads. Don't close
WEB-server socket manually, use FD_CLOEXEC fcntl instead.
@sysoleg sysoleg mentioned this pull request Mar 4, 2016
@sysoleg
Copy link
Contributor Author

sysoleg commented Mar 5, 2016

Looks like this patch breaks wdctl restart functionality:
[5][Sat Mar 5 06:54:54 2016][1058](gateway.c:389) Creating web server on 192.168.1.2:2060 [3][Sat Mar 5 06:54:54 2016][1058](gateway.c:391) Could not create web server: Address already in use
WEB-server socket must be closed explicitly when restarting process from wdctl.

At any rate we should avoid using free() in child code when forking. Maybe don't set FD_CLOEXEC, but convert fd_list_t from linked list to static array? fd_list_t is very simple structure so it can be just array. The only user for it is WEB-server.

@sysoleg
Copy link
Contributor Author

sysoleg commented Mar 5, 2016

Something like this.

free() is not safe in fork()ed child when using threads. Convert
fd_list_t linked list to plain array in order to avoid free() call.
@KazukiShimada
Copy link

Hi, devs.
Could you please review and merge this if it's OK?
Thanks in advance.

@mhaas mhaas merged commit 8a2e54e into wifidog:master May 22, 2016
@mhaas
Copy link
Contributor

mhaas commented May 22, 2016

Ah, great, I merged with the obsolete commit message..

Just to make sure I got this correctly: we cannot use free() in threads so we use a statically allocated array of webserver fds. Since it's not a (dynamically allocated) linked list, we don't have to call free - correct?

@mhaas
Copy link
Contributor

mhaas commented May 22, 2016

@oleg-umnik @KazukiShimada appreciate the contribution, by the way.

@sysoleg
Copy link
Contributor Author

sysoleg commented May 23, 2016

@mhaas, yes, that's right. Thanks!

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