Re: [PATCH] scripts: handle BrokenPipeError for python scripts

[Nicolas Schier]

On Thu, Jan 12, 2023 at 10:55:45AM -0800 Nick Desaulniers wrote:
> On Wed, Jan 11, 2023 at 6:30 PM Masahiro Yamada <masahiroy@xxxxxxxxxx> 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

Hi Masahiro,

should it be "... seems to be the ..."?  

Reviewed-by: Nicolas Schier <nicolas@xxxxxxxxx>

> > 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@xxxxxxxxxx>
> 
> >
> > [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@xxxxxxxxxxxx>
> > Signed-off-by: Masahiro Yamada <masahiroy@xxxxxxxxxx>
> > ---
> >
> >  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

Attachment: signature.asc
Description: PGP signature