Re: [PATCH v4] tools: jobserver: Prevent deadlock caused by incorrect jobserver configuration and enhance error reporting

[Jonathan Corbet]

Changbin Du <changbin.du@xxxxxxxxxx> writes:

> When using GNU Make's jobserver feature in kernel builds, a bug in MAKEFLAGS
> propagation caused "--jobserver-auth=r,w" to reference an unintended file
> descriptor. This led to infinite loops in jobserver-exec's os.read() calls
> due to empty token.
>
> My shell opened /etc/passwd for some reason without closing it, and as a
> result, all child processes inherited this fd 3.
>
> $ ls -l /proc/self/fd
> total 0
> lrwx------ 1 changbin changbin 64 Dec 25 13:03 0 -> /dev/pts/1
> lrwx------ 1 changbin changbin 64 Dec 25 13:03 1 -> /dev/pts/1
> lrwx------ 1 changbin changbin 64 Dec 25 13:03 2 -> /dev/pts/1
> lr-x------ 1 changbin changbin 64 Dec 25 13:03 3 -> /etc/passwd
> lr-x------ 1 changbin changbin 64 Dec 25 13:03 4 -> /proc/1421383/fd
>
> In this case, the `make` should open a new file descriptor for jobserver
> control, but clearly, it did not do so and instead still passed fd 3 as
> "--jobserver-auth=3,4" in MAKEFLAGS. (The version of my gnu make is 4.3)
>
> This update ensures robustness against invalid jobserver configurations,
> even when `make` incorrectly pass non-pipe file descriptors.
>  * Rejecting empty reads to prevent infinite loops on EOF.
>  * Clearing `self.jobs` to avoid writing to incorrect files if invalid tokens
>    are detected.
>  * Printing detailed error messages to stderr to inform the user.
>
> Cc: Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx>
> Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx>
> Signed-off-by: Changbin Du <changbin.du@xxxxxxxxxx>

So I've applied this; it appears to work, though I can't really test the
error case that it is intended to fix.

However, it adds a new warning to a standard "make htmldocs" build:

> jobserver: warning: IndexError('list index out of range')

You have not added the exception, you just put in a print that brought
it to the surface.

The warning comes from JobserverExec::open(), for an exception that
appears to be expected.  This is the sort of use of exceptions that has
made me almost swear off them entirely in Python - it's a huge try block
that is using exceptions to hide a bunch of the assumptions and logic.
I'll be posting a patch shortly to remove this non-exceptional exception
case.

Thanks,

jon