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.
Suggested-by: Rasmus Villemoes <[email protected]>
Link: https://lore.kernel.org/lkml/[email protected]
Signed-off-by: Kees Cook <[email protected]>
---
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.
+ 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)
+except (KeyError, IndexError, ValueError, IOError, OSError) as e:
+ print(e, file=sys.stderr)
# Any missing environment strings or bad fds should result in just
# using the default specified parallelism.
print(default)
--
2.17.1
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.
> Suggested-by: Rasmus Villemoes <[email protected]>
> Link: https://lore.kernel.org/lkml/[email protected]
> Signed-off-by: Kees Cook <[email protected]>
> ---
> 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.
Rasmus
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 <[email protected]>
> > Link: https://lore.kernel.org/lkml/[email protected]
> > Signed-off-by: Kees Cook <[email protected]>
> > ---
> > 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