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

afprog: manual cloexec #3416

Merged
merged 2 commits into from
Sep 14, 2020
Merged

Conversation

Kokan
Copy link
Collaborator

@Kokan Kokan commented Sep 3, 2020

Parent waits for child to close all fd before exec.

Fixes #3409.

modules/afprog/afprog.c Outdated Show resolved Hide resolved
modules/afprog/afprog.c Outdated Show resolved Hide resolved
modules/afprog/afprog.c Outdated Show resolved Hide resolved
@kira-syslogng
Copy link
Contributor

Build SUCCESS

@Kokan Kokan force-pushed the afprog-manual-cloexec branch 2 times, most recently from ea26525 to 81bbf6f Compare September 3, 2020 12:34
@alltilla
Copy link
Collaborator

alltilla commented Sep 3, 2020

Do we still need the CLOEXEC flag on the fds? In other words: can exec() be called from any other place, than the program dest/src? Maybe our java or python bindings do such thing.

We might be able to remove the CLOEXEC flag, because it is replaced by this new logic.

@kira-syslogng
Copy link
Contributor

Build SUCCESS

@Kokan
Copy link
Collaborator Author

Kokan commented Sep 4, 2020

Both java and python languages bindings uses threads and not forking. In spite of those modules there is still fork used in syslog-ng.

  1. The very first when syslog-ng stars and spawn a supervisor for itself (as needed); which is not an issue as that is created before anything else, so there is interesting open fds.
  2. command-hooks also uses fork (but it directly calls system which yields fork+exec)

Not to mention that we could have a lib that does fork+exec without us knowing (sure not likely), imho better to keep them.

@Kokan Kokan changed the title WIP: afprog: manual cloexec afprog: manual cloexec Sep 4, 2020
alltilla
alltilla previously approved these changes Sep 4, 2020
Copy link
Collaborator

@alltilla alltilla left a comment

Choose a reason for hiding this comment

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

This works perfectly, thanks! 👍

@@ -145,13 +184,20 @@ afprogram_popen(AFProgramProcessInfo *process_info, GIOCondition cond, gint *fd)
close(msg_pipe[0]);
close(msg_pipe[1]);

_close_all_fd();
Copy link
Collaborator

Choose a reason for hiding this comment

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

With the latest iteration, nothing ensures that sync-fd is closed after all inherited sockets, so you would need to to dup2() it to fd 3 to make sure. If we inherited fd 3, that would simply close it before the iteration, but then syncfd is closed last.

Copy link
Collaborator

@gaborznagy gaborznagy left a comment

Choose a reason for hiding this comment

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

Besides Bazsi's comment to use fd 3 approve from my side.

@kira-syslogng
Copy link
Contributor

Build SUCCESS

Copy link
Collaborator

@alltilla alltilla left a comment

Choose a reason for hiding this comment

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

Thanks!

@kira-syslogng
Copy link
Contributor

Build SUCCESS

Copy link
Collaborator

@gaborznagy gaborznagy left a comment

Choose a reason for hiding this comment

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

Thanks!

@gaborznagy gaborznagy merged commit 660783b into syslog-ng:master Sep 14, 2020
@Kokan Kokan added this to the syslog-ng-3.30 milestone Sep 15, 2020
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.

network: syslog-ng fails to bind() after config revert
5 participants