Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AddressSanitizer: Skip tests directly in Python, not with external config #90791

Closed
vstinner opened this issue Feb 4, 2022 · 15 comments
Closed
Labels
3.11 only security fixes tests Tests in the Lib/test dir

Comments

@vstinner
Copy link
Member

vstinner commented Feb 4, 2022

BPO 46633
Nosy @vstinner, @pablogsal
PRs
  • bpo-46633: Skip tests on ASAN and/or MSAN builds #31632
  • [3.10] bpo-46633: Skip tests on ASAN and/or MSAN builds (GH-31632) #31634
  • [3.9] bpo-46633: Skip tests on ASAN and/or MSAN builds (GH-31632) (GH-31634) #31644
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2022-03-02.17:13:40.631>
    created_at = <Date 2022-02-04.10:09:11.092>
    labels = ['tests', '3.11']
    title = 'AddressSanitizer: Skip tests directly in Python, not with external config'
    updated_at = <Date 2022-03-02.17:13:40.630>
    user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2022-03-02.17:13:40.630>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2022-03-02.17:13:40.631>
    closer = 'vstinner'
    components = ['Tests']
    creation = <Date 2022-02-04.10:09:11.092>
    creator = 'vstinner'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 46633
    keywords = ['patch']
    message_count = 11.0
    messages = ['412499', '412501', '412506', '414247', '414248', '414250', '414262', '414266', '414370', '414379', '414380']
    nosy_count = 2.0
    nosy_names = ['vstinner', 'pablogsal']
    pr_nums = ['31632', '31634', '31644']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue46633'
    versions = ['Python 3.11']

    Linked PRs

    @vstinner
    Copy link
    Member Author

    vstinner commented Feb 4, 2022

    It seems like bpo-45200: "Address Sanitizer: libasan dead lock in pthread_create() (test_multiprocessing_fork.test_get() hangs)" is not fixed yet.

    In the GitHub Action job, test_multiprocessing_fork is skipped because it's too slow, so the job doesn't hang.

    Yesterday, I modified the ASAN buildbot to double its timeout from 20 to 40 minutes:
    python/buildmaster-config@5a37411

    But it's useful, when it hangs, it hangs forever. Exmaple on the AMD64 Arch Linux Asan Debug 3.9 buildbot (with the new config):

    ---
    (test.test_multiprocessing_fork.WithProcessesTestPicklingConnections) ... ok
    Timeout (0:35:00)!
    ---
    https://buildbot.python.org/all/#/builders/588/builds/332

    Tests are tuned for ASAN, but the configuration is copied and inconsistent between the GitHub Actions job and the buildbot configuration.

    I propose to move this configuration directly into Python.

    test_decimal.py checks for "-fsanitize=address" in CFLAGS and skip some tests if it's present.

    @vstinner vstinner added 3.11 only security fixes tests Tests in the Lib/test dir labels Feb 4, 2022
    @vstinner
    Copy link
    Member Author

    vstinner commented Feb 4, 2022

    Notes:

    • test.support has check_sanitizer() function. Tests using it:

      • test_crypt
      • test_idle
      • test_tix
      • test_tk
      • test_ttk_guionly
    • test_decimal also checks for memory sanitizer and skip some tests if it's present:

    MEMORY_SANITIZER = (
        '-fsanitize=memory' in _cflags or
        '--with-memory-sanitizer' in _config_args
    )   
    • test_faulthandler suppress crash report, similir to support.SuppressCrashReport, directly in the C code used by tests: faulthandler_suppress_crash_report() function.

    • Objects/obmalloc.c checks "#if __has_feature(address_sanitizer)": is ASAN enabled?

    • GitHub Action config: .github/workflows/build.yml. Skipped tests:

      • test___all__
      • test_concurrent_futures
      • test_multiprocessing_fork
      • test_multiprocessing_forkserver
      • test_multiprocessing_spawn
      • test_peg_generator
      • test_tools

    Comment:
    ---
    # Skip test_tools test_peg_generator test_concurrent_futures because
    # there are too slow: between 5 and 20 minutes on this CI.

    # Skip multiprocessing and concurrent.futures tests which are affected by
    # bpo-45200 bug: libasan dead lock in pthread_create().

    # test___all__ is skipped because importing some modules directly can trigger
    # known problems with ASAN (like tk or crypt).
    ---

    • Buildbot configuration: UnixAsanBuild class of master/custom/factories.py. Options:
      ASAN_OPTIONS='detect_leaks=0:allocator_may_return_null=1:handle_segv=0'

    With the comment:
    ---
    # See https://bugs.python.org/issue42985 for more context on why
    # SIGSEGV is ignored on purpose.
    ---

    Skipped tests:

    • test_ctypes
    • test_capi
    • test_crypt
    • test_decimal
    • test_faulthandler
    • test_interpreters

    With the comment:
    ---
    # These tests are currently raising false positives or are interfering with the ASAN mechanism,
    # so we need to skip them unfortunately.
    ---

    @pablogsal
    Copy link
    Member

    I have recently added some decorators in test.
    support to deactivate tests if running under the sanitizers.

    @vstinner
    Copy link
    Member Author

    vstinner commented Mar 1, 2022

    I created a PR to no longer skip tests in buildbots:
    python/buildmaster-config#314

    @vstinner
    Copy link
    Member Author

    vstinner commented Mar 1, 2022

    Status with the 2 pending PRs:

    Tests only skipped on ASAN:

    • _test_multiprocessing

    Skip on ASAN and MSAN:

    • test___all__
    • test_concurrent_futures
    • test_crypt
    • test_decimal.test_maxcontext_exact_arith()
    • test_idle
    • test_peg_generator
    • test_tix
    • test_tk
    • test_tools
    • test_ttk_guionly

    No longer skipped:

    • test_capi
    • test_ctypes
    • test_faulthandler
    • test_interpreters

    @vstinner
    Copy link
    Member Author

    vstinner commented Mar 1, 2022

    I created bpo-46887: ./Programs/_freeze_module fails with MSAN: Uninitialized value was created by an allocation of 'stat.i'.

    @vstinner
    Copy link
    Member Author

    vstinner commented Mar 1, 2022

    """
    No longer skipped:

    • test_capi
    • test_ctypes
    • test_faulthandler
    • test_interpreters
      """

    I built Python manually with:

    ./configure --with-pydebug CC=clang LD=clang --with-address-sanitizer

    These tests no longer with ASAN_OPTIONS used on our CI:

    ASAN_OPTIONS='detect_leaks=0:allocator_may_return_null=1:handle_segv=0' ./python -m test (...)

    @vstinner
    Copy link
    Member Author

    vstinner commented Mar 1, 2022

    New changeset 9204bb7 by Victor Stinner in branch 'main':
    bpo-46633: Skip tests on ASAN and/or MSAN builds (GH-31632)
    9204bb7

    @vstinner
    Copy link
    Member Author

    vstinner commented Mar 2, 2022

    New changeset 9326445 by Victor Stinner in branch '3.10':
    [3.10] bpo-46633: Skip tests on ASAN and/or MSAN builds (GH-31632) (GH-31634)
    9326445

    @vstinner
    Copy link
    Member Author

    vstinner commented Mar 2, 2022

    New changeset 359bc39 by Victor Stinner in branch '3.9':
    [3.10] bpo-46633: Skip tests on ASAN and/or MSAN builds (GH-31632) (GH-31634) (GH-31644)
    359bc39

    @vstinner
    Copy link
    Member Author

    vstinner commented Mar 2, 2022

    python/buildmaster-config#314

    I merged this PR as well. The new buildbot configuration will be deployed soon.

    @vstinner vstinner closed this as completed Mar 2, 2022
    @vstinner vstinner closed this as completed Mar 2, 2022
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @vstinner
    Copy link
    Member Author

    vstinner commented May 5, 2022

    Lib/test/test_asyncio/test_subprocess.py and Lib/test/test_distutils.py are now skipped on ASAN tests by the commit: f6dd14c (cc @gpshead)

    @gpshead
    Copy link
    Member

    gpshead commented May 6, 2022

    FWIW I disabled those two because they triggered ASAN on Ubuntu 20.04's gcc 9.4 and in github's CI (which might use that?). They do not trigger ASAN on more recent Debian bullseye or Ubuntu 22.04 based systems with a more recent gcc.

    In the _posixsubprocess.c case it was at a callsite that has ~25 parameters (ugh) and just gained one more. But the asan assertion would only trigger during one consistent but seemingly random test_asyncio.test_subprocess test rather than all the time; so if there is a bug in the ASAN implementation itself it isn't clear what it was.

    @vstinner
    Copy link
    Member Author

    I enabled test_faulthandler on ASAN: commit 58f9c63.

    vstinner added a commit to vstinner/cpython that referenced this issue Aug 22, 2023
    Only skip modules and tests related to X11 on ASAN builds: run other
    tests with ASAN.
    
    Use print(flush=True) to see output earlier when it's redirected to a
    pipe.
    vstinner added a commit to vstinner/cpython that referenced this issue Aug 22, 2023
    * Only skip modules and tests related to X11 on ASAN builds: run
      other tests with ASAN.
    * Use print(flush=True) to see output earlier when it's redirected to
      a pipe.
    * Update issue reference: replace bpo-46633 with pythongh-90791.
    vstinner added a commit to vstinner/cpython that referenced this issue Aug 22, 2023
    * Cleanup libregrtest code logging ASAN_OPTIONS.
    * Fix a typo on "ASAN_OPTIONS" vs "MSAN_OPTIONS".
    vstinner added a commit to vstinner/cpython that referenced this issue Aug 22, 2023
    * Cleanup libregrtest code logging ASAN_OPTIONS.
    * Fix a typo on "ASAN_OPTIONS" vs "MSAN_OPTIONS".
    vstinner added a commit that referenced this issue Aug 22, 2023
    * Only skip modules and tests related to X11 on ASAN builds: run
      other tests with ASAN.
    * Use print(flush=True) to see output earlier when it's redirected to
      a pipe.
    * Update issue reference: replace bpo-46633 with gh-90791.
    @vstinner
    Copy link
    Member Author

    I enabled more tests in test___all__: commit a541e01

    vstinner added a commit that referenced this issue Aug 22, 2023
    * Cleanup libregrtest code logging ASAN_OPTIONS.
    * Fix a typo on "ASAN_OPTIONS" vs "MSAN_OPTIONS".
    vstinner added a commit to vstinner/cpython that referenced this issue Sep 2, 2023
    )
    
    * Cleanup libregrtest code logging ASAN_OPTIONS.
    * Fix a typo on "ASAN_OPTIONS" vs "MSAN_OPTIONS".
    
    (cherry picked from commit 3a1ac87)
    vstinner added a commit that referenced this issue Sep 3, 2023
    …108820)
    
    * Revert "[3.11] gh-101634: regrtest reports decoding error as failed test (#106169) (#106175)"
    
    This reverts commit d5418e9.
    
    * Revert "[3.11] bpo-46523: fix tests rerun when `setUp[Class|Module]` fails (GH-30895) (GH-103342)"
    
    This reverts commit ecb09a8.
    
    * Revert "gh-95027: Fix regrtest stdout encoding on Windows (GH-98492)"
    
    This reverts commit b2aa28e.
    
    * Revert "[3.11] gh-94026: Buffer regrtest worker stdout in temporary file (GH-94253) (GH-94408)"
    
    This reverts commit 0122ab2.
    
    * Revert "Run Tools/scripts/reindent.py (GH-94225)"
    
    This reverts commit f0f3a42.
    
    * Revert "gh-94052: Don't re-run failed tests with --python option (GH-94054)"
    
    This reverts commit 1347607.
    
    * Revert "[3.11] gh-84461: Fix Emscripten umask and permission issues (GH-94002) (GH-94006)"
    
    This reverts commit 1073184.
    
    * gh-93353: regrtest checks for leaked temporary files (#93776)
    
    When running tests with -jN, create a temporary directory per process
    and mark a test as "environment changed" if a test leaks a temporary
    file or directory.
    
    (cherry picked from commit e566ce5)
    
    * gh-93353: Fix regrtest for -jN with N >= 2 (GH-93813)
    
    (cherry picked from commit 36934a1)
    
    * gh-93353: regrtest supports checking tmp files with -j2 (#93909)
    
    regrtest now also implements checking for leaked temporary files and
    directories when using -jN for N >= 2. Use tempfile.mkdtemp() to
    create the temporary directory. Skip this check on WASI.
    
    (cherry picked from commit 4f85cec)
    
    * gh-84461: Fix Emscripten umask and permission issues (GH-94002)
    
    - Emscripten's default umask is too strict, see
      emscripten-core/emscripten#17269
    - getuid/getgid and geteuid/getegid are stubs that always return 0
      (root). Disable effective uid/gid syscalls and fix tests that use
      chmod() current user.
    - Cannot drop X bit from directory.
    
    (cherry picked from commit 2702e40)
    
    * gh-94052: Don't re-run failed tests with --python option (#94054)
    
    (cherry picked from commit 0ff7b99)
    
    * Run Tools/scripts/reindent.py (#94225)
    
    Reindent files which were not properly formatted (PEP 8: 4 spaces).
    
    Remove also some trailing spaces.
    
    (cherry picked from commit e87ada4)
    
    * gh-94026: Buffer regrtest worker stdout in temporary file (GH-94253)
    
    Co-authored-by: Victor Stinner <[email protected]>
    (cherry picked from commit 199ba23)
    
    * gh-96465: Clear fractions hash lru_cache under refleak testing (GH-96689)
    
    Automerge-Triggered-By: GH:zware
    (cherry picked from commit 9c8f379)
    
    * gh-95027: Fix regrtest stdout encoding on Windows (#98492)
    
    On Windows, when the Python test suite is run with the -jN option,
    the ANSI code page is now used as the encoding for the stdout
    temporary file, rather than using UTF-8 which can lead to decoding
    errors.
    
    (cherry picked from commit ec1f6f5)
    
    * gh-98903: Test suite fails with exit code 4 if no tests ran (#98904)
    
    The Python test suite now fails wit exit code 4 if no tests ran. It
    should help detecting typos in test names and test methods.
    
    * Add "EXITCODE_" constants to Lib/test/libregrtest/main.py.
    * Fix a typo: "NO TEST RUN" becomes "NO TESTS RAN"
    
    (cherry picked from commit c76db37)
    
    * gh-100086: Add build info to test.libregrtest (#100093)
    
    The Python test runner (libregrtest) now logs Python build information like
    "debug" vs "release" build, or LTO and PGO optimizations.
    
    (cherry picked from commit 3c89202)
    
    * bpo-46523: fix tests rerun when `setUp[Class|Module]` fails (#30895)
    
    Co-authored-by: Jelle Zijlstra <[email protected]>
    Co-authored-by: Łukasz Langa <[email protected]>
    (cherry picked from commit 9953860)
    
    * gh-82054: allow test runner to split test_asyncio to execute in parallel by sharding. (#103927)
    
    This runs test_asyncio sub-tests in parallel using sharding from Cinder. This suite is typically the longest-pole in runs because it is a test package with a lot of further sub-tests otherwise run serially. By breaking out the sub-tests as independent modules we can run a lot more in parallel.
    
    After porting we can see the direct impact on a multicore system.
    
    Without this change:
      Running make test is 5 min 26 seconds
    With this change:
      Running make test takes 3 min 39 seconds
    
    That'll vary based on system and parallelism. On a `-j 4` run similar to what CI and buildbot systems often do, it reduced the overall test suite completion latency by 10%.
    
    The drawbacks are that this implementation is hacky and due to the sorting of the tests it obscures when the asyncio tests occur and involves changing CPython test infrastructure but, the wall time saved it is worth it, especially in low-core count CI runs as it pulls a long tail. The win for productivity and reserved CI resource usage is significant.
    
    Future tests that deserve to be refactored into split up suites to benefit from are test_concurrent_futures and the way the _test_multiprocessing suite gets run for all start methods. As exposed by passing the -o flag to python -m test to get a list of the 10 longest running tests.
    
    ---------
    
    Co-authored-by: Carl Meyer <[email protected]>
    Co-authored-by: Gregory P. Smith <[email protected]> [Google, LLC]
    (cherry picked from commit 9e011e7)
    
    * Display the sanitizer config in the regrtest header. (#105301)
    
    Display the sanitizers present in libregrtest.
    
    Having this in the CI output for tests with the relevant environment
    variable displayed will help make it easier to do what we need to
    create an equivalent local test run.
    
    (cherry picked from commit 852348a)
    
    * gh-101634: regrtest reports decoding error as failed test (#106169)
    
    When running the Python test suite with -jN option, if a worker stdout
    cannot be decoded from the locale encoding report a failed testn so the
    exitcode is non-zero.
    
    (cherry picked from commit 2ac3eec)
    
    * gh-108223: test.pythoninfo and libregrtest log Py_NOGIL (#108238)
    
    Enable with --disable-gil --without-pydebug:
    
        $ make pythoninfo|grep NOGIL
        sysconfig[Py_NOGIL]: 1
    
        $ ./python -m test
        ...
        == Python build: nogil debug
        ...
    
    (cherry picked from commit 5afe0c1)
    
    * gh-90791: test.pythoninfo logs ASAN_OPTIONS env var (#108289)
    
    * Cleanup libregrtest code logging ASAN_OPTIONS.
    * Fix a typo on "ASAN_OPTIONS" vs "MSAN_OPTIONS".
    
    (cherry picked from commit 3a1ac87)
    
    * gh-108388: regrtest splits test_asyncio package (#108393)
    
    Currently, test_asyncio package is only splitted into sub-tests when
    using command "./python -m test". With this change, it's also
    splitted when passing it on the command line:
    "./python -m test test_asyncio".
    
    Remove the concept of "STDTESTS". Python is now mature enough to not
    have to bother with that anymore. Removing STDTESTS simplify the
    code.
    
    (cherry picked from commit 174e9da)
    
    * regrtest computes statistics (#108793)
    
    test_netrc, test_pep646_syntax and test_xml_etree now return results
    in the test_main() function.
    
    Changes:
    
    * Rewrite TestResult as a dataclass with a new State class.
    * Add test.support.TestStats class and Regrtest.stats_dict attribute.
    * libregrtest.runtest functions now modify a TestResult instance
      in-place.
    * libregrtest summary lists the number of run tests and skipped
      tests, and denied resources.
    * Add TestResult.has_meaningful_duration() method.
    * Compute TestResult duration in the upper function.
    * Use time.perf_counter() instead of time.monotonic().
    * Regrtest: rename 'resource_denieds' attribute to 'resource_denied'.
    * Rename CHILD_ERROR to MULTIPROCESSING_ERROR.
    * Use match/case syntadx to have different code depending on the
      test state.
    
    Co-authored-by: Alex Waygood <[email protected]>
    (cherry picked from commit d4e534c)
    
    * gh-108822: Add Changelog entry for regrtest statistics (#108821)
    
    ---------
    
    Co-authored-by: Christian Heimes <[email protected]>
    Co-authored-by: Zachary Ware <[email protected]>
    Co-authored-by: Nikita Sobolev <[email protected]>
    Co-authored-by: Joshua Herman <[email protected]>
    Co-authored-by: Gregory P. Smith <[email protected]>
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.11 only security fixes tests Tests in the Lib/test dir
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants