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

catatoinit: close fds >= 3 #14

Merged

cyphar merged 1 commit into openSUSE:master from giuseppe:close-fds-ge-than-3

Sep 15, 2021

Contributor

giuseppe commented Sep 2, 2021

close any additional fd that was already leaked into the child
process.

Closes: #12

Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com

giuseppe marked this pull request as draft

September 2, 2021 12:30

alexlarsson reviewed

View reviewed changes

catatonit.c Show resolved Hide resolved

cyphar added the lgtm/need 2 label

alexlarsson reviewed

View reviewed changes

catatonit.c Outdated Show resolved Hide resolved

giuseppe force-pushed the close-fds-ge-than-3 branch from 6ba3b11 to 733c5b3 Compare

September 2, 2021 12:45

alexlarsson reviewed

View reviewed changes

catatonit.c Outdated Show resolved Hide resolved

giuseppe force-pushed the close-fds-ge-than-3 branch from 733c5b3 to 0b81b8b Compare

September 2, 2021 12:46

giuseppe marked this pull request as ready for review

September 2, 2021 12:47

alexlarsson reviewed

View reviewed changes

catatonit.c Outdated Show resolved Hide resolved

giuseppe force-pushed the close-fds-ge-than-3 branch 2 times, most recently from 047ac8e to c4563e1 Compare

September 2, 2021 13:00

alexlarsson reviewed

View reviewed changes

catatonit.c Show resolved Hide resolved

giuseppe force-pushed the close-fds-ge-than-3 branch from c4563e1 to 1ecdb97 Compare

September 2, 2021 13:18

alexlarsson commented Sep 2, 2021

lgtm

Contributor Author

giuseppe commented Sep 14, 2021

@cyphar are you fine with this change?

cyphar requested changes

View reviewed changes

catatonit.c Outdated Show resolved Hide resolved

catatonit.c Outdated Show resolved Hide resolved

catatonit.c Outdated Show resolved Hide resolved

catatonit.c Outdated Show resolved Hide resolved

catatonit.c Outdated

@@ @@ -361,6 +439,8 @@ int main(int argc, char **argv) @@
               		bail("self-check that pid1 (%d) was spawned failed: %m", pid1);
               	debug("pid1 (%d) spawned: %s", pid1, argv[0]);
+              	close_fds_ge_than(3, sfd);

Member

cyphar Sep 15, 2021

We obviously shouldn't bail here, but some kind of debug output if closing failed wouldn't hurt.

Contributor Author

giuseppe Sep 15, 2021

there is already a warn for each close() that fails in the fallback code. Should we add another warn here?

Member

cyphar Sep 15, 2021 •

edited

Loading

If close_range() is supported you wouldn't get any indication the close failed right? It could just be a debug message so you don't get duplicate warnings.

EDIT: Actually maybe the warning for each fd should be a debug message, and a warning from calling this just to make the warnings consistent.

Member

cyphar commented Sep 15, 2021 •

edited

Loading

Sorry, for some reason I didn't see this -- I think GitHub has given up on showing me new notifications from smaller repos. 😬

EDIT: Oh, for some reason my watch notifications for this repo weren't set to "everything". Sorry about that...

giuseppe force-pushed the close-fds-ge-than-3 branch from 1ecdb97 to 326e481 Compare

September 15, 2021 09:41

Contributor Author

giuseppe commented Sep 15, 2021

Sorry, for some reason I didn't see this -- I think GitHub has given up on showing me new notifications from smaller repos. grimacing

EDIT: Oh, for some reason my watch notifications for this repo weren't set to "everything". Sorry about that...

no worries! Thanks for the review.

I've added a check for close_range in configure.ac and go through the syscall only when it is noy already defined in the standard library.

I've tested both build modes on Fedora 34, where close_range() is not available, and on Fedora Rawhide where it is defined.

cyphar reviewed

View reviewed changes

catatonit.c Outdated Show resolved Hide resolved

cyphar mentioned this pull request

catatoinit: rewrite LISTEN_PID to the child process #15

Merged


          catatoinit: close fds >= 3

9c60e7d

close any additional fd that was already leaked into the child
process.

Closes: openSUSE#12

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe force-pushed the close-fds-ge-than-3 branch from 326e481 to 9c60e7d Compare

September 15, 2021 10:59

cyphar approved these changes

View reviewed changes

Member

cyphar left a comment

LGTM.

cyphar added lgtm/need 1 and removed lgtm/need 2 labels

Member

cyphar commented Sep 15, 2021 •

edited

Loading

Ah I just noticed an embarrassing bug while testing this -- catatonit doesn't set the /dev/tty handle to O_CLOEXEC when foregrounding the child process. Will fix that too...

cyphar merged commit 0d74f13 into openSUSE:master

cyphar mentioned this pull request

catatonit keeps a mount directory open, preventing unmounting #28

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels