Opened 5 years ago

Closed 5 years ago

#9168 closed bug (fixed)

reading/writing blocking FDs over FD_SETSIZE is broken

Reported by: rwbarton Owned by:
Priority: normal Milestone: 7.8.3
Component: libraries/base Version: 7.8.1
Keywords: Cc: hvr, ekmett
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s):
Wiki Page:

Description

In GHC.IO.FD when reading from or writing to a blocking FD we first check (using our C function fdReady) whether the underlying fd is ready for read/write, in an attempt to avoid blocking the current OS thread. On POSIX this check is done using select, with no test for whether the fd exceeds FD_SETSIZE, causing a write out of bounds and various bad consequences.

Also, while readRawBufferPtr checks the error status of fdReady, readRawBufferPtrNoBlock, writeRawBufferPtr, writeRawBufferPtrNoBlock do not, making this issue harder to diagnose.

I suggest that fdReady use poll(2) where available. I can prepare a set of patches if needed.

Attachments (1)

Broken.hs (632 bytes) - added by rwbarton 5 years ago.

Download all attachments as: .zip

Change History (7)

Changed 5 years ago by rwbarton

Attachment: Broken.hs added

comment:1 Changed 5 years ago by rwbarton

A reproducer, based on https://github.com/smurphy8/broken-acid-test. Note: you need a max open files ulimit of more than about 1030 to reproduce the bug with this program.

comment:2 Changed 5 years ago by rwbarton

Oh, and if poll(2) is not available and so we have to fall back to select, and we call fdReady on an fd exceeding FD_SETSIZE, we should raise an exception, of course.

comment:3 Changed 5 years ago by nicolast

Can't the changes from #4934 (9fd507e5758f4141ac2619f0db57136bcab035c6) trigger this as well?

comment:4 in reply to:  3 Changed 5 years ago by slyfox

Replying to nicolast:

Can't the changes from #4934 (9fd507e5758f4141ac2619f0db57136bcab035c6) trigger this as well?

You mean regression introduction?

I think not, as fd sanity is checked right before main select() call. As it's a single-threaded runtime case no haskell tasks should be able to be pushed to blocked queue (with very big FD).

comment:5 Changed 5 years ago by slyfox

Simplest "fix" (as done in non-threaded IOmanager):

--- a/libraries/base/cbits/inputReady.c
+++ b/libraries/base/cbits/inputReady.c
@@ -25,7 +25,9 @@ fdReady(int fd, int write, int msecs, int isSock)
 	int maxfd, ready;
 	fd_set rfd, wfd;
 	struct timeval tv;
-	
+	if ((fd >= (int)FD_SETSIZE) || (fd < 0)) {
+	    return -1;
+	}
 	FD_ZERO(&rfd);
 	FD_ZERO(&wfd);
         if (write) {

comment:6 Changed 5 years ago by thoughtpolice

Milestone: 7.8.3
Resolution: fixed
Status: newclosed

Merged into 7.8.3.

Note: See TracTickets for help on using tickets.