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

Fix double close on ubivol descriptor #1419

Closed
wants to merge 1 commit into from

Conversation

tvdstaaij
Copy link

The g_output_stream_close call was introduced at some point after the casync functionality for UBI was introduced (#501). This close call is fine for the copy_raw_image branch, but for the casync branch it causes the update to fail:

rauc[250]: Installation error: Failed updating slot rootfs.1: Error closing file descriptor: Bad file descriptor

This was also mentioned in #1200. However it was not investigated there but instead sidestepped by using a different update approach.

Investigation with strace shows that the desciptor is closed twice:

290   openat(AT_FDCWD, "/dev/ubi0_1", O_WRONLY|O_EXCL|O_LARGEFILE <unfinished ...>
290   <... openat resumed>)             = 8
[snip]
290   close(8 <unfinished ...>
290   <... close resumed>)              = 0
290   close(8 <unfinished ...>
290   <... close resumed>)              = -1 EBADF (Bad file descriptor)

This can be explained by the documentation of
g_subprocess_launcher_close():

Closes all the file descriptors previously passed to the object with
g_subprocess_launcher_take_fd(), g_subprocess_launcher_take_stderr_fd(), etc.
(...)
This function is called automatically when the #GSubprocessLauncher is disposed

The g_output_stream_close call was introduced at some point after the
casync functionality for UBI was introduced (rauc#501). This close call is
fine for the copy_raw_image branch, but for the casync branch it causes
the update to fail:

rauc[250]: Installation error: Failed updating slot rootfs.1: \
    Error closing file descriptor: Bad file descriptor

This was also mentioned in rauc#1200. However it was not investigated there
but instead sidestepped by using a different update approach.

Investigation with strace shows that the desciptor is closed twice:

290   openat(AT_FDCWD, "/dev/ubi0_1", O_WRONLY|O_EXCL|O_LARGEFILE <unfinished ...>
290   <... openat resumed>)             = 8
[snip]
290   close(8 <unfinished ...>
290   <... close resumed>)              = 0
290   close(8 <unfinished ...>
290   <... close resumed>)              = -1 EBADF (Bad file descriptor)

This can be explained by the documentation of
g_subprocess_launcher_close():

> Closes all the file descriptors previously passed to the object with
> g_subprocess_launcher_take_fd(), g_subprocess_launcher_take_stderr_fd(), etc.
> (...)
> This function is called automatically when the #GSubprocessLauncher is disposed

Signed-off-by: Tim van der Staaij <git@tim.vanderstaaij.email>
@jluebbe
Copy link
Member

jluebbe commented May 14, 2024

The intention with the explicit close is to be able to catch more errors (some of which are only reported on close()). So perhaps a better way is to pass a dup()ed FD to g_subprocess_launcher_take_stdout_fd()?

Sadly this code is not covered by the CI, as we didn't want to build a custom version of casync systemd/casync#227.

@tvdstaaij
Copy link
Author

The intention with the explicit close is to be able to catch more errors (some of which are only reported on close()). So perhaps a better way is to pass a dup()ed FD to g_subprocess_launcher_take_stdout_fd()?

Ah, I wasn't aware of the existence of dup(). I submitted a new PR.

@tvdstaaij tvdstaaij closed this May 15, 2024
@jluebbe
Copy link
Member

jluebbe commented May 15, 2024

Note that you can force-push to your branch to update this PR, which keeps the discussion in one place. :)

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.

None yet

2 participants