Skip to content

Undefined behaviour in main and 3.11 #96678

Closed
Closed
Undefined behaviour in main and 3.11#96678
Labels
3.11only security fixes3.12only security fixestype-bugAn unexpected behavior, bug, or errortype-crashA hard crash of the interpreter, possibly with a core dump

Description

@matthiasgoergens
matthiasgoergens
opened on Sep 8, 2022

I ran the sanitizers again, and found a few more instances of undefined behaviour, mostly around bit-shifting of signed integers and arithmetic with NULL pointers.

export CC="clang"
export ASAN_OPTIONS=detect_leaks=0
configure --with-assertions --with-address-sanitizer --with-trace-refs --with-undefined-behavior-sanitizer --with-pydebug
nice make -j8
make test

I put some asserts to demonstrate the undefined behaviour into pull requests for main (matthiasgoergens#18) and 3.11 (matthiasgoergens#19).

More information about my environment:

$ clang --version
clang version 14.0.6
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin

Activity

  1. matthiasgoergens
    added
    type-bugAn unexpected behavior, bug, or error
    on Sep 8, 2022
  2. matthiasgoergens
    changed the title [-]Undefined behaviours in main and 3.11[/-] [+]Undefined behaviours in `main` and 3.11[/+] on Sep 8, 2022
  3. matthiasgoergens
    changed the title [-]Undefined behaviours in `main` and 3.11[/-] [+]Undefined behaviour in `main` and 3.11[/+] on Sep 8, 2022

  4. kumaraditya303 commented on Sep 8, 2022

    @kumaraditya303
    kumaraditya303
    on Sep 8, 2022
    Contributor

    Can you post a summary of "arithmetic with NULL pointers" here? Thanks

  5. kumaraditya303
    added
    3.11only security fixes
    3.12only security fixes
    type-crashA hard crash of the interpreter, possibly with a core dump
    on Sep 8, 2022

  6. matthiasgoergens commented on Sep 8, 2022

    @matthiasgoergens
    matthiasgoergens
    on Sep 8, 2022
    ContributorAuthor

    @kumaraditya303 In ceval.c, the assert is my addition.

        /* Pack other positional arguments into the *args argument */
    if (co->co_flags & CO_VARARGS) {
        PyObject *u = NULL;
        assert(args != NULL);
        u = _PyTuple_FromArraySteal(args + n, argcount - n);
        if (u == NULL) {
            goto fail_post_positional;
        }
        assert(localsplus[total_args] == NULL);
        localsplus[total_args] = u;
    }

    There's also an example in Modules/_testcapimodule.c but that's only a test, I guess.

  7. matthiasgoergens commented on Sep 8, 2022

    @matthiasgoergens
    matthiasgoergens
    on Sep 8, 2022
    ContributorAuthor

    #96672 is also vaguely related.

  8. kumaraditya303 commented on Sep 8, 2022

    @kumaraditya303
    kumaraditya303
    on Sep 8, 2022
    Contributor

    cc @pablogsal This seems to be introduced in #89419

  9. pablogsal commented on Sep 8, 2022

    @pablogsal
    pablogsal
    on Sep 8, 2022
    Member

    I am a bit surprised by this because we have a USAN buildbot that has not detected anything:

    https://buildbot.python.org/all/#/builders/964

  10. pablogsal commented on Sep 8, 2022

    @pablogsal
    pablogsal
    on Sep 8, 2022
    Member

    These are the parameters, just in case we want to compare:

    https://github.com/python/buildmaster-config/blob/8484356e8a0854084e186d9c9eb47b3b0c6eb628/master/custom/factories.py#L311-L315

  11. matthiasgoergens commented on Sep 8, 2022

    @matthiasgoergens
    matthiasgoergens
    on Sep 8, 2022
    ContributorAuthor

    @pablogsal I added some more info about what compiler I am using.

  12. pablogsal commented on Sep 8, 2022

    @pablogsal
    pablogsal
    on Sep 8, 2022
    Member

    @pablogsal I added some more info about what compiler I am using.

    Can you see if you reproduce these warnings using the exact flags the buildbot is using?

  13. matthiasgoergens commented on Sep 8, 2022

    @matthiasgoergens
    matthiasgoergens
    on Sep 8, 2022
    ContributorAuthor

    I'll try that later today.

  14. 33 remaining items

  15. matthiasgoergens commented on Sep 11, 2022

    @matthiasgoergens
    matthiasgoergens
    on Sep 11, 2022
    ContributorAuthor

    As a practical test, we can take the asserts I added in the PRs mentioned in the text of the issue, convert them into something that triggers in release mode (eg a normal if and a printf or abort) and run them under the same conditions as release mode.

    I don't know why the sanitizer doesn't pick any of this up. At least it doesn't pick any of this up when the build bot runs it, neither in release mode nor in the debug mode.

    I ran my sanitizer locally with different flags. See above.

    (I'm only on my phone right now, so can't run these experiments until later today.)

  16. pablogsal commented on Sep 11, 2022

    @pablogsal
    pablogsal
    on Sep 11, 2022
    Member

    I don't know why the sanitizer doesn't pick any of this up. At least it doesn't pick any of this up when the build bot runs it, neither in release mode nor in the debug mode.

    Answering that question is very important, as it may reveal very interesting things such as problems in the running builder or what is the difference in behaviour between your builds and the builder and if the differences matte or not.

    neither in release mode nor in the debug mode.

    The sanitizer only runs in release mode in the build bots

  17. matthiasgoergens commented on Sep 11, 2022

    @matthiasgoergens
    matthiasgoergens
    on Sep 11, 2022
    ContributorAuthor

    The sanitizer only runs in release mode in the build bots

    Oh, thanks. I must have misunderstood something.

    Of course, our general point about not actually picking up many instances of UB still stands.

    Btw, the config that you linked to that had the sanitizer enabled didn't enable optimisations as far as I can tell, hence I didn't see it as release mode.

  18. matthiasgoergens commented on Sep 12, 2022

    @matthiasgoergens
    matthiasgoergens
    on Sep 12, 2022
    ContributorAuthor

    @pablogsal @markshannon @kumaraditya303

    Good news! I found the rootcause of why my debug build with all sanitisers turned up showed undefined behaviour, but they didn't show up in the build bot.

    Most invocations of configure add -fwrapv to the compiler options. But --with-pydebug removes that.

    From clang's documentation about -fwrapv

    Treat signed integer overflow as two’s complement

    GCC is a bit more verbose, but still rather vague:

    This option instructs the compiler to assume that signed arithmetic overflow of addition, subtraction and multiplication wraps around using twos-complement representation. This flag enables some optimizations and disables others.

    It turns out that arithmetic on the null-pointer also becomes defined with -fwrapv. At least my experiments point in that direction. The only thing I could find is some discussion on the netbsd kernel mailing list that seems to confirm this.

    If the above is true, then all the instances of UB that I found are not actually UB in the effective C-dialect we are using for release builds.

    Now we have (at least) two options:

    • fix --with-pydebug to also add -fwrapv for consistency.
    • remove -fwrapv from the release builds.

    The former is the minimal change and safe. The latter enables a lot of C compiler optimizations, and my experiments running the sanitisers suggests that we are already nearly -fno-wrapv-ready. In general, we might want to look into -fstrict-overflow, too.

    I say we should do the former straight away, and in the longer term consider implementing the latter (after running benchmarks and weighing the pros and cons etc).

  19. pablogsal
    added a commit that references this issue on Sep 13, 2022

    gh-96678: Avoid undefined behaviour in test (#96672)

    6ba686d
  20. miss-islington
    added 4 commits that reference this issue on Sep 13, 2022

    pythongh-96678: Avoid undefined behaviour in test (pythonGH-96672)

    b570747

    pythongh-96678: Fix UB of null pointer arithmetic (pythonGH-96782)

    8aa6e81

    gh-96678: Fix UB of null pointer arithmetic (GH-96782)

    81e36f3

    pythongh-96678: Fix UB of null pointer arithmetic (pythonGH-96782)

    69b8ad7
  21. pablogsal
    added a commit that references this issue on Sep 13, 2022

    gh-96678: Avoid undefined behaviour in test (GH-96672) (#96795)

    cc8b006
  22. miss-islington
    added 2 commits that reference this issue on Sep 13, 2022

    gh-96678: Fix UB of null pointer arithmetic (GH-96782)

    f2d7fa8

    gh-96678: Fix UB of null pointer arithmetic (GH-96782)

    d357f71

  23. matthiasgoergens commented on Sep 14, 2022

    @matthiasgoergens
    matthiasgoergens
    on Sep 14, 2022
    ContributorAuthor

    I am closing this issue for now, because thanks to -fwrapv we don't actually have any UB in the release builds. Only when building --with-pydebug or with an explicit optimization option on the command line.

  24. matthiasgoergens
    closed this as completedon Sep 14, 2022
  25. Repository owner moved this from Todo to Done in Release and Deferred blockers 🚫on Sep 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    3.11only security fixes3.12only security fixestype-bugAn unexpected behavior, bug, or errortype-crashA hard crash of the interpreter, possibly with a core dump

    Projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions