On Thu, Nov 21, 2019 at 08:41:01AM +0100, Rasmus Villemoes wrote: > On 21/11/2019 01.03, Kees Cook wrote: > > Setting non-blocking via a local copy of the jobserver file descriptor > > is safer than just assuming the writer on the original fd is prepared > > for it to be non-blocking. > > This is a bit inaccurate. The fd referring to the write side of the pipe > is always blocking - it has to be, due to the protocol requiring you to > write back the tokens you've read, so you can't just drop a token on the > floor. But it's also rather moot, since the pipe will never hold > anywhere near 4096 bytes, let alone a (linux) pipe's default capacity of > 64K. > > But what we cannot do is change the mode of the open file description to > non-blocking for the read side, in case the parent make (or some sibling > process that has also inherited the same "struct file") expects it to be > blocking. Ah! This explains my confusion over what you were trying to tell me before. I thought you meant the other end of the pipe, which seemed crazy. You mean the other jobserver readers (i.e. "make" itself) who have the same shared _reader_ fd. This is exactly what you said, but I was too dense. :) I'll fix this up! > > > Suggested-by: Rasmus Villemoes <linux@xxxxxxxxxxxxxxxxxx> > > Link: https://lore.kernel.org/lkml/44c01043-ab24-b4de-6544-e8efd153e27a@xxxxxxxxxxxxxxxxxx > > Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx> > > --- > > scripts/jobserver-count | 15 +++++++-------- > > 1 file changed, 7 insertions(+), 8 deletions(-) > > > > diff --git a/scripts/jobserver-count b/scripts/jobserver-count > > index 6e15b38df3d0..a68a04ad304f 100755 > > --- a/scripts/jobserver-count > > +++ b/scripts/jobserver-count > > @@ -12,12 +12,6 @@ default="1" > > if len(sys.argv) > 1: > > default=sys.argv[1] > > > > -# Set non-blocking for a given file descriptor. > > -def nonblock(fd): > > - flags = fcntl.fcntl(fd, fcntl.F_GETFL) > > - fcntl.fcntl(fd, fcntl.F_SETFL, flags | os.O_NONBLOCK) > > - return fd > > - > > # Extract and prepare jobserver file descriptors from envirnoment. > > try: > > # Fetch the make environment options. > > @@ -31,8 +25,13 @@ try: > > # Parse out R,W file descriptor numbers and set them nonblocking. > > fds = opts[0].split("=", 1)[1] > > reader, writer = [int(x) for x in fds.split(",", 1)] > > - reader = nonblock(reader) > > -except (KeyError, IndexError, ValueError, IOError): > > + # Open a private copy of reader to avoid setting nonblocking > > + # on an unexpecting writer. > > s/writer/reader/ > > > + reader = os.open("/proc/self/fd/%d" % (reader), os.O_RDONLY) > > + flags = fcntl.fcntl(reader, fcntl.F_GETFL) > > + fcntl.fcntl(reader, fcntl.F_SETFL, flags | os.O_NONBLOCK) > > I think you can just specify O_NONBLOCK in the open() call so you avoid > those two fcntls. Hah. Yes indeed. -- Kees Cook
