PR#3793: SSS_CERT: Close file descriptors after executing p11_child - SSSD/sssd

SSSD / sssd

#3793 SSS_CERT: Close file descriptors after executing p11_child

Merged 6 years ago by jhrozek. Opened 6 years ago by lslebodn.

SSSD/ lslebodn/sssd ssh_fd_leak into master

SSS_CERT: Close file descriptors after executing p11_child

Lukas Slebodnik • 6 years ago

3e0c7c5

file modified

		`@@ -255,6 +255,9 @@`
		`int ret;`
		`bool valid = false;`

		`+ PIPE_FD_CLOSE(state->io->read_from_child_fd);`
		`+ PIPE_FD_CLOSE(state->io->write_to_child_fd);`
		`+`
		`if (WIFEXITED(child_status)) {`
		`if (WEXITSTATUS(child_status) != 0) {`
		`DEBUG(SSSDBG_OP_FAILURE,`

lslebodn commented 6 years ago

We can call cert_to_ssh_key_step from cert_to_ssh_key_done and thus
p11_child can be executed more time. We created pipes for each call
but destructor for state->io can close just last one.

It's better to manually close pipes with macro PIPE_FD_CLOSE.
that macro set file descriptor to -1 and destructor will not try
to close them 2nd time. Destructor will cover just edge cases.

rebased onto 3e0c7c5

6 years ago

jhrozek commented 6 years ago

So I was working on the issue as well because I didn't know you are also looking at it -- please, next time, add a comment to the BZ or just assign the BZ to yourself so that we avoid double work.

Nonetheless, my approach was to still use the destructor, but allocate and free the state->io during each step. I thought this was safer because the destructor would be called regardless of future changes. I wasn't sure if I should go a step further and use a step_ctx as a membe of state that would be allocated during step and the io structure would be a child of the state.

You can view the WIP version here -- https://github.com/jhrozek/sssd/commit/b47084312037be60a8d91a386c100ca6125f2f42

It also prevents the bug in my testing.

lslebodn commented 6 years ago

Nonetheless, my approach was to still use the destructor, but allocate and free the state->io during each step. I thought this was safer because the destructor would be called regardless of future changes. I wasn't sure if I should go a step further and use a step_ctx as a membe of state that would be allocated during step and the io structure would be a child of the state.
You can view the WIP version here -- https://github.com/jhrozek/sssd/commit/b47084312037be60a8d91a386c100ca6125f2f42
It also prevents the bug in my testing.

I would say there are really small differences between approaches. I am not sure what kind of future changes do you mean because there si still talloc destructor which will close file descriptors.

I did a micro optimisation with avoiding allocation for every child. Which is negligible comparing to fork + exec.

So it is up to you to decide which approach is cleaner.

Thank you for review.

jhrozek commented 6 years ago

I guess they are equivalent, then. First come first served.

ACK

jhrozek commented 6 years ago

(Running CI and Coverity before pushing)

jhrozek commented 6 years ago

Commit a76f96a fixes this pull-request