GNOME Bugzilla – Bug 682819
EINTR-harden all the things
Last modified: 2013-03-18 03:55:24 UTC
Noticed by code inspection, when auditing some of my code for EINTR handling.
Created attachment 222573 [details] [review] gstdio: Harden g_open() against EINTR
Comment on attachment 222573 [details] [review] gstdio: Harden g_open() against EINTR oh EINTR...
Attachment 222573 [details] pushed as ce976bc - gstdio: Harden g_open() against EINTR
What about all the close calls?
Created attachment 222799 [details] [review] Handle EINTR for a few more write() calls
(In reply to comment #4) > What about all the close calls? So there's a *lot* of them... I could add say g_unix_close_noeintr()?
To be clear, the reason this matters is that someone was complaining about some GNOME-related software crashing after a resume. The error message had "Interrupted system call" in it. I think it's something like while suspending, the kernel will offline CPUs and freeze processes, and this may end up generating an EINTR visible to userspace where one normally wouldn't. Kind of evil to have bugs that only appear when suspending, so we should be defensive. It doesn't hurt to fix other Unix implementations too.
(In reply to comment #6) > (In reply to comment #4) > > What about all the close calls? > > So there's a *lot* of them... I could add say g_unix_close_noeintr()? how would that help, as opposed to g_close()? it seems like we would want to check everywhere, since otherwise you're leaking a fd, right? oh, meanwhile, I just noticed that by default on unix: #define g_open open
close is not wrapped with g_ like open is, because we generally only have wrappers because we need to do special things on win32 with all the syscalls that take pathnames, and close doesn't. In general its not really possible to wrap all of these safely, as e.g. the exact definition of "struct stat" and "stat" and other things may wary depending on some defines. Also, it seems like EINTR should not be handled on close on Linux (although it needs to be on HPUX, dafuq!?): https://sites.google.com/site/michaelsafyan/software-engineering/checkforeintrwheninvokingclosethinkagain http://utcc.utoronto.ca/~cks/space/blog/unix/CloseEINTR
(In reply to comment #9) > close is not wrapped with g_ like open is, because we generally only have > wrappers because we need to do special things on win32 with all the syscalls > that take pathnames, and close doesn't. Historically, yes, but as of Colin's earlier commit on this bug, g_open() now also does EINTR-retrying on unix. (Except that it doesn't actually get used because of #defines in the header.) I think we could usefully use EINTR-retrying read() and write() too... > In general its not really possible to wrap all of these safely, as e.g. the > exact definition of "struct stat" and "stat" and other things may wary > depending on some defines. Is that still true in modern times? But anyway, we could use a macro. (And the stat man page doesn't list EINTR as a possibility anyway, though maybe that's a bug.)
It seems like stat may actually return EINTR, at least on solaris. http://www.linuxmisc.com/3-solaris/2716b3b80cbf8053.htm http://lists.gnu.org/archive/html/bug-make/2002-11/msg00022.html And yeah, off_t and struct stat size is still dependent on LFS defines. Might make sense to wrap read/write too to avoid this, although these calls are not all that common in glib.
Created attachment 234420 [details] [review] 0001-GUnixOutputStream-Handle-EINTR-for-close.patch
Comment on attachment 234420 [details] [review] 0001-GUnixOutputStream-Handle-EINTR-for-close.patch According to the link in comment 9, this is explicitly wrong, at least on Linux.
Comment on attachment 222799 [details] [review] Handle EINTR for a few more write() calls assuming this still applies, it looks good, although didn't we add a helper function for this at some point?
(In reply to comment #13) > (From update of attachment 234420 [details] [review]) > According to the link in comment 9, this is explicitly wrong, at least on > Linux. Holy shit. I missed that in this bug. That's *awful*. Truly, staggeringly, amazingly awful. Arrrrgh.
Yes. Yes it is.
Comment on attachment 222799 [details] [review] Handle EINTR for a few more write() calls Hm, no I don't see a helper anywhere. It's probably worth adding one though.
Created attachment 234428 [details] [review] 0001-Add-g_unix_close_fd-use-it.patch
Review of attachment 234428 [details] [review]: Maybe we could just call it g_close()?
some of those places (eg, glocalfileinputstream) are not unix-specific
Created attachment 234440 [details] [review] 0001-Add-g_close-use-it.patch
Review of attachment 234440 [details] [review]: ::: glib/gstdio.c @@ +874,3 @@ + } + return TRUE; +} This seem to override errno in case of an error. I mean, its saved and reused in the g_file_error_from_errno call, but after return of g_close you can't rely on errno. And it seems to me that a bunch of the other code relies on errno being right, like in gio/glocalfileinputstream.c
Created attachment 234732 [details] [review] 0001-Add-g_close-use-it.patch Correctly re-set errno, update documentation to reflect this.
Review of attachment 234732 [details] [review]: looks good
Leaving this bug open, there's still some incorrect write() calls, though mostly in the tests.
*** Bug 564138 has been marked as a duplicate of this bug. ***
*** This bug has been marked as a duplicate of bug 656492 ***