Message-ID: <CAKwvOd=k=veNG=Q-bZxX4iX4q14e2ozO_sHZhdCz1+BwBxq=cQ@mail.gmail.com>
Date:   Thu, 12 Jan 2023 10:55:45 -0800
From:   Nick Desaulniers <ndesaulniers@...gle.com>
To:     Masahiro Yamada <masahiroy@...nel.org>
Cc:     linux-kbuild@...r.kernel.org, linux-kernel@...r.kernel.org,
        Kees Cook <keescook@...omium.org>,
        Nathan Chancellor <nathan@...nel.org>,
        Tom Rix <trix@...hat.com>, llvm@...ts.linux.dev
Subject: Re: [PATCH] scripts: handle BrokenPipeError for python scripts

On Wed, Jan 11, 2023 at 6:30 PM Masahiro Yamada <masahiroy@...nel.org> wrote:
>
> In the follow-up of commit fb3041d61f68 ("kbuild: fix SIGPIPE error
> message for AR=gcc-ar and AR=llvm-ar"), Kees Cook pointed out that
> tools should _not_ catch their own SIGPIPEs [1] [2].
>
> Based on his feedback, LLVM was fixed [3].
>
> However, Python's default behavior is to show noisy bracktrace when
> SIGPIPE is sent. So, scripts written in Python are basically in the
> same situation as the buggy llvm tools.
>
> Example:
>
>   $ make -s allnoconfig
>   $ make -s allmodconfig
>   $ scripts/diffconfig .config.old .config | head -n1
>   -ALIX n
>   Traceback (most recent call last):
>     File "/home/masahiro/linux/scripts/diffconfig", line 132, in <module>
>       main()
>     File "/home/masahiro/linux/scripts/diffconfig", line 130, in main
>       print_config("+", config, None, b[config])
>     File "/home/masahiro/linux/scripts/diffconfig", line 64, in print_config
>       print("+%s %s" % (config, new_value))
>   BrokenPipeError: [Errno 32] Broken pipe
>
> Python documentatin [4] notes how to make scripts die immediately and

typo: s/documentatin/documentation/

> silently:
>
>   """
>   Piping output of your program to tools like head(1) will cause a
>   SIGPIPE signal to be sent to your process when the receiver of its
>   standard output closes early. This results in an exception like
>   BrokenPipeError: [Errno 32] Broken pipe. To handle this case,
>   wrap your entry point to catch this exception as follows:
>
>     import os
>     import sys
>
>     def main():
>         try:
>             # simulate large output (your code replaces this loop)
>             for x in range(10000):
>                 print("y")
>             # flush output here to force SIGPIPE to be triggered
>             # while inside this try block.
>             sys.stdout.flush()
>         except BrokenPipeError:
>             # Python flushes standard streams on exit; redirect remaining output
>             # to devnull to avoid another BrokenPipeError at shutdown
>             devnull = os.open(os.devnull, os.O_WRONLY)
>             os.dup2(devnull, sys.stdout.fileno())
>             sys.exit(1)  # Python exits with error code 1 on EPIPE
>
>     if __name__ == '__main__':
>         main()
>
>   Do not set SIGPIPE’s disposition to SIG_DFL in order to avoid
>   BrokenPipeError. Doing that would cause your program to exit
>   unexpectedly whenever any socket connection is interrupted while
>   your program is still writing to it.
>   """
>
> Currently, tools/perf/scripts/python/intel-pt-events.py seems the
> only script that fixes the issue that way.
>
> tools/perf/scripts/python/compaction-times.py uses another approach
> signal.signal(signal.SIGPIPE, signal.SIG_DFL) but the Python
> documentation clearly says "Don't do it".
>
> I cannot fix all Python scripts since there are so many.
> I fixed some in the scripts/ directory.

That's ok; "Rome wasn't built in a day." This is a good start!
Thank you for the patch!
Reviewed-by: Nick Desaulniers <ndesaulniers@...gle.com>

>
> [1]: https://lore.kernel.org/all/202211161056.1B9611A@keescook/
> [2]: https://github.com/llvm/llvm-project/issues/59037
> [3]: https://github.com/llvm/llvm-project/commit/4787efa38066adb51e2c049499d25b3610c0877b
> [4]: https://docs.python.org/3/library/signal.html#note-on-sigpipe
>
> Cc: Kees Cook <keescook@...omium.org>
> Signed-off-by: Masahiro Yamada <masahiroy@...nel.org>
> ---
>
>  scripts/checkkconfigsymbols.py         | 13 ++++++++++++-
>  scripts/clang-tools/run-clang-tools.py | 21 ++++++++++++++-------
>  scripts/diffconfig                     | 16 ++++++++++++++--
>  3 files changed, 40 insertions(+), 10 deletions(-)
>
> diff --git a/scripts/checkkconfigsymbols.py b/scripts/checkkconfigsymbols.py
> index 217d21abc86e..36c920e71313 100755
> --- a/scripts/checkkconfigsymbols.py
> +++ b/scripts/checkkconfigsymbols.py
> @@ -115,7 +115,7 @@ def parse_options():
>      return args
>
>
> -def main():
> +def print_undefined_symbols():
>      """Main function of this module."""
>      args = parse_options()
>
> @@ -467,5 +467,16 @@ def parse_kconfig_file(kfile):
>      return defined, references
>
>
> +def main():
> +    try:
> +        print_undefined_symbols()
> +    except BrokenPipeError:
> +        # Python flushes standard streams on exit; redirect remaining output
> +        # to devnull to avoid another BrokenPipeError at shutdown
> +        devnull = os.open(os.devnull, os.O_WRONLY)
> +        os.dup2(devnull, sys.stdout.fileno())
> +        sys.exit(1)  # Python exits with error code 1 on EPIPE
> +
> +
>  if __name__ == "__main__":
>      main()
> diff --git a/scripts/clang-tools/run-clang-tools.py b/scripts/clang-tools/run-clang-tools.py
> index 56f2ec8f0f40..3266708a8658 100755
> --- a/scripts/clang-tools/run-clang-tools.py
> +++ b/scripts/clang-tools/run-clang-tools.py
> @@ -61,14 +61,21 @@ def run_analysis(entry):
>
>
>  def main():
> -    args = parse_arguments()
> +    try:
> +        args = parse_arguments()
>
> -    lock = multiprocessing.Lock()
> -    pool = multiprocessing.Pool(initializer=init, initargs=(lock, args))
> -    # Read JSON data into the datastore variable
> -    with open(args.path, "r") as f:
> -        datastore = json.load(f)
> -        pool.map(run_analysis, datastore)
> +        lock = multiprocessing.Lock()
> +        pool = multiprocessing.Pool(initializer=init, initargs=(lock, args))
> +        # Read JSON data into the datastore variable
> +        with open(args.path, "r") as f:
> +            datastore = json.load(f)
> +            pool.map(run_analysis, datastore)
> +    except BrokenPipeError:
> +        # Python flushes standard streams on exit; redirect remaining output
> +        # to devnull to avoid another BrokenPipeError at shutdown
> +        devnull = os.open(os.devnull, os.O_WRONLY)
> +        os.dup2(devnull, sys.stdout.fileno())
> +        sys.exit(1)  # Python exits with error code 1 on EPIPE
>
>
>  if __name__ == "__main__":
> diff --git a/scripts/diffconfig b/scripts/diffconfig
> index d5da5fa05d1d..43f0f3d273ae 100755
> --- a/scripts/diffconfig
> +++ b/scripts/diffconfig
> @@ -65,7 +65,7 @@ def print_config(op, config, value, new_value):
>          else:
>              print(" %s %s -> %s" % (config, value, new_value))
>
> -def main():
> +def show_diff():
>      global merge_style
>
>      # parse command line args
> @@ -129,4 +129,16 @@ def main():
>      for config in new:
>          print_config("+", config, None, b[config])
>
> -main()
> +def main():
> +    try:
> +        show_diff()
> +    except BrokenPipeError:
> +        # Python flushes standard streams on exit; redirect remaining output
> +        # to devnull to avoid another BrokenPipeError at shutdown
> +        devnull = os.open(os.devnull, os.O_WRONLY)
> +        os.dup2(devnull, sys.stdout.fileno())
> +        sys.exit(1)  # Python exits with error code 1 on EPIPE
> +
> +
> +if __name__ == '__main__':
> +    main()
> --
> 2.34.1
>


-- 
Thanks,
~Nick Desaulniers

Powered by blists - more mailing lists