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
