handle EINTR when using read()/write()

[lieff]

Code which uses g_file_read/g_file_write do not handle EINTR. To make code signal safe we need handle it within them.

[speidy]

I think the caller should be responsible for the while loop. its just a wrapper.

[lieff]

For now g_file_read/g_file_write callers do not handle EINTR/EAGAIN. I agree that EAGAIN should be handled by caller, but EINTR must be handled every time, so less code if it moved to wrapper.
Also here #728, @metalefty said that errors should be handled in g_file_read(), that’s why I made this PR.

[metalefty]

It makes sense keepingg_file_read as a wrapper of read system call. What about making another function to wrap g_file_read and handle errors in the function?

[lieff]

May be, but in some way it must be handled. I would have a case in web server where EINTR occurred even when we do not send any signals to process (may be some daemon do). So its real danger not to handle EINTR.

[metalefty]

@speidy What do you think of #729 (comment)?

[lieff]

Any update on it? I look how it handled in FreeRDP. It uses BIO_read functions from openssl winch handles EINTR and several other errors in BIO_fd_should_retry->BIO_fd_non_fatal_error. So I think it's normal to handle it in wrapper.

[metalefty]

I'll make some change based on your commit.

[metalefty]

I'm now inclined to merge this as-is.
@jsorg71 @speidy @proski Any opinions?

[jsorg71]

These functions are for read and write to a file. This has nothing to do with openssl read and write for the socket.

[metalefty]

Yeah, read()/write() may cause EINTR error, it needs to be handled. This does that.

[lieff]

BIO interface is universal, it supports regular files in bss_fd.c and bss_file.c. So, not a big difference about wrapper discussion. About why it helps #728, seems file IO error somewhere leads to something that reflects on ssl stream handling.

[metalefty]

@jsorg71 said in #728

I think we can also merge the EINTR fix. It does not make anything worse. As a rule we can handle EINTR in os_calls.c for all the functions that can return it. We can not handle EAGAIN(and the like) in os_calls.c as the higher levels are expecting them.

@jsorg71 it's OK for you to merge this as-is?
speidy wants to keep g_file_read()/g_file_write() as a simple wrapper of read()/write().
Handling EINTR in another wrapper wrapper is also an option.

[lieff]

Note that there can be more places without handled EINTR, not only for read()\write() for example, but for other syscalls like open(). Closer look needed.

[jsorg71]

One example of how handling EINTR in os_calls.c can be undesired is if a file read is blocking forever and someone sends a term signal to xrdp. Currently, the read would break, that thread loop would go around, see that is_term is set, and process would exit. If we handle EINTR is os_calls.c, xrdp would not close down when receiving the term signal until the read is done, which might be never.
Of course, file read should never block that long.

[cro]

coughNFScough

[lieff]

Yes, its a problem. For correct SIGTERM we must handle EINTR in every use place and make correct thread shutdown chain. Or possible solution for this is to handle SIGTERM in main thread (and do not use blocking calls from it), and handle possible "unclean" shutdown of stuck threads (or create separate watchdog thread, so main thread can use blocking IO). Btw openssl BIO stuck forever too in this case, so, it may stuck in 3rd party libs anyway.