#369 Avoid I/O operation on closed file when calling multiple backend hooks
Closed a year ago by churchyard. Opened a year ago by churchyard.
rpms/ churchyard/pyproject-rpm-macros fix_reentrant_hook  into  rawhide

file modified
+5 -1
@@ -10,7 +10,7 @@ 

  #   Increment Y and reset Z when new macros or features are added

  #   Increment Z when this is a bugfix or a cosmetic change

  # Dropping support for EOL Fedoras is *not* considered a breaking change

- Version:        1.6.3

+ Version:        1.6.4

  Release:        1%{?dist}

  

  # Macro files
@@ -147,6 +147,10 @@ 

  

  

  %changelog

+ * Fri Mar 31 2023 Miro Hrončok <mhroncok@redhat.com> - 1.6.4-1

+ - Avoid I/O operation on closed file when calling multiple backend hooks

+ - Fixes: rhbz#2183519

+ 

  * Mon Feb 13 2023 Lumír Balhar <lbalhar@redhat.com> - 1.6.3-1

  - Remove .dist-info directory at the end of %%pyproject_buildrequires

  - An incomplete .dist-info directory in $PWD can confuse tests in %%check

file modified
+34 -26
@@ -4,7 +4,6 @@ 

  import sys

  import importlib.metadata

  import argparse

- import tempfile

  import traceback

  import contextlib

  import json
@@ -46,24 +45,33 @@ 

  from pyproject_convert import convert

  

  

- @contextlib.contextmanager

- def hook_call():

-     """Context manager that records all stdout content (on FD level)

-     and prints it to stderr at the end, with a 'HOOK STDOUT: ' prefix."""

-     tmpfile = io.TextIOWrapper(

+ def fake_stdout():

+     """Context manager that holds a fake stdout file pointer

+     to be used in hook_call().

+     It's essential that this context manager is only used once and all

+     hook_call()s are called from within.

+     See https://bugzilla.redhat.com/2183519#c2 for rationale."""

+     return io.TextIOWrapper(

          tempfile.TemporaryFile(buffering=0),

          encoding='utf-8',

          errors='replace',

          write_through=True,

      )

  

+ 

+ @contextlib.contextmanager

+ def hook_call(fp):

+     """Context manager that records all stdout content (on FD level)

+     in the given file pointer and prints it to stderr at the end,

+     with a 'HOOK STDOUT: ' prefix.

+     Use this context manager repeatedly inside one fake_stdout()."""

      stdout_fd = 1

      stdout_fd_dup = os.dup(stdout_fd)

      stdout_orig = sys.stdout

  

      # begin capture

-     sys.stdout = tmpfile

-     os.dup2(tmpfile.fileno(), stdout_fd)

+     sys.stdout = fp

+     os.dup2(fp.fileno(), stdout_fd)

  

      try:

          yield
@@ -72,12 +80,10 @@ 

          sys.stdout = stdout_orig

          os.dup2(stdout_fd_dup, stdout_fd)

  

-         tmpfile.seek(0)  # rewind

-         for line in tmpfile:

+         fp.seek(0)  # rewind

+         for line in fp:

              print_err('HOOK STDOUT:', line, end='')

  

-         tmpfile.close()

- 

  

  def guess_reason_for_invalid_requirement(requirement_str):

      if ':' in requirement_str:
@@ -282,10 +288,10 @@ 

      return backend_module

  

  

- def generate_build_requirements(backend, requirements):

+ def generate_build_requirements(backend, requirements, *, hook_stdout):

      get_requires = getattr(backend, 'get_requires_for_build_wheel', None)

      if get_requires:

-         with hook_call():

+         with hook_call(hook_stdout):

              new_reqs = get_requires()

          requirements.extend(new_reqs, source='get_requires_for_build_wheel')

          requirements.check(source='get_requires_for_build_wheel')
@@ -296,7 +302,7 @@ 

      return {k: message.get_all(k, ()) for k in ('Requires', 'Requires-Dist')}

  

  

- def generate_run_requirements_hook(backend, requirements):

+ def generate_run_requirements_hook(backend, requirements, *, hook_stdout):

      hook_name = 'prepare_metadata_for_build_wheel'

      prepare_metadata = getattr(backend, hook_name, None)

      if not prepare_metadata:
@@ -306,7 +312,7 @@ 

              'Use the provisional -w flag to build the wheel and parse the metadata from it, '

              'or use the -R flag not to generate runtime dependencies.'

          )

-     with hook_call():

+     with hook_call(hook_stdout):

          dir_basename = prepare_metadata('.')

      with open(dir_basename + '/METADATA') as metadata_file:

          for key, requires in requires_from_metadata_file(metadata_file).items():
@@ -347,11 +353,11 @@ 

              raise RuntimeError('Could not find *.dist-info/METADATA in built wheel.')

  

  

- def generate_run_requirements(backend, requirements, *, build_wheel, wheeldir):

+ def generate_run_requirements(backend, requirements, *, build_wheel, wheeldir, hook_stdout):

      if build_wheel:

          generate_run_requirements_wheel(backend, requirements, wheeldir)

      else:

-         generate_run_requirements_hook(backend, requirements)

+         generate_run_requirements_hook(backend, requirements, hook_stdout=hook_stdout)

  

  

  def generate_tox_requirements(toxenv, requirements):
@@ -434,14 +440,16 @@ 

                      source=f'requirements file {req_file.name}'

                  )

              requirements.check(source='all requirements files')

-         if use_build_system:

-             backend = get_backend(requirements)

-             generate_build_requirements(backend, requirements)

-         if toxenv:

-             include_runtime = True

-             generate_tox_requirements(toxenv, requirements)

-         if include_runtime:

-             generate_run_requirements(backend, requirements, build_wheel=build_wheel, wheeldir=wheeldir)

+         with fake_stdout() as hook_stdout:

+             if use_build_system:

+                 backend = get_backend(requirements)

+                 generate_build_requirements(backend, requirements, hook_stdout=hook_stdout)

+             if toxenv:

+                 include_runtime = True

+                 generate_tox_requirements(toxenv, requirements)

+             if include_runtime:

+                 generate_run_requirements(backend, requirements, hook_stdout=hook_stdout,

+                                           build_wheel=build_wheel, wheeldir=wheeldir)

      except EndPass:

          return

  

The previous implementation could leak to:

ValueError: I/O operation on closed file.

When hook_call() was used multiple times.

Setuptools 65.6+ uses logging: https://setuptools.pypa.io/en/latest/history.html#v65-6-0
That change exposed this bug.

The bug happened like this:

  1. hook_call() creates a temporary file and makes stdout point to it
  2. in hook_call(): setuptools initiates a global logger, storing a reference to the temporary file
  3. hook_call() dumps and closes the temporary file
  4. subsequent hook_call() creates another temporary file
  5. in hook_call(): setuptools logs and tries to write to the closed file stored in (2)
  6. hook_call() dumps and close the empty temporary file created in (4)

The fix makes hook_call() take a file argument
and ensures all hook_calls are called with the same temporary file.

I failed to add a new test for this to test_pyproject_buildrequires.py. We could possibly test this in one of the spec files, but it would be nasty.

rebased onto f8e160d

a year ago

Build succeeded.
https://fedora.softwarefactory-project.io/zuul/buildset/05b1fd187a8344e299d6bfe98cb96db2

IMO, the alternative is much cleaner.

Pull-Request has been closed by churchyard

a year ago