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

Refresh Screen Provided By curses.wrapper Causes Seg Fault (macOS, xcode 15 Apple supplied ncurses 6.0 breakage) #109617

Open
timway opened this issue Sep 20, 2023 · 32 comments
Labels
OS-mac type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@timway
Copy link

timway commented Sep 20, 2023

Crash report

What happened?

MacOS began pushing out updates to XCode Command Line Tools to install 15.0 recently. Upon updating I began having issues with curses. This happens with the Python provided by Apple. I'm not aware of the best way to communicate this issue to Apple, hopefully someone here knows who to ping or is watching.

Save the below as curses-segfault.py:

import curses

def main(stdscr):
    stdscr.refresh()

curses.wrapper(main)

Run the script in zsh in MacOS Terminal via:

/usr/bin/python3 curses-segfault.py

An easy way to see if you got the update is via the terminal by running:

softwareupdate --history

CPython versions tested on:

3.9

Operating systems tested on:

macOS

Output from running 'python -VV' on the command line:

python3 -VV Python 3.9.6 (default, Aug 11 2023, 19:44:49) [Clang 15.0.0 (clang-1500.0.40.1)]

Linked PRs

@timway timway added the type-crash A hard crash of the interpreter, possibly with a core dump label Sep 20, 2023
@brandtbucher
Copy link
Member

brandtbucher commented Sep 20, 2023

I've been encountering this on main too after updating (the test_curses tests in our suite seem to trigger it). One really annoying side-effect is that it breaks your terminal after the crash:

% ./python.exe -m test --multiprocess 1 --use curses --verbose test_curses
0:00:00 load avg: 1.27 [1/1/1] test_curses process crashed (Exit code -11)
                                                                          test_has_extended_color_support (test.test_curses.MiscTests.test_has_extended_color_support) ... ok
    test_ncurses_version (test.test_curses.MiscTests.test_ncurses_version) ... ncurses_version = curses.ncurses_version(major=6, minor=0, patch=20150808)
                                                                                                                                                         ok
                                                                                                                                                           test_update_lines_cols (test.test_curses.MiscTests.test_update_lines_cols) ... ok
                                                                   test_alt (test.test_curses.TestAscii.test_alt) ... ok
                                                                                                                        test_ascii (test.test_curses.TestAscii.test_ascii) ... ok
        test_controlnames (test.test_curses.TestAscii.test_controlnames) ... ok
                                                                               test_ctrl (test.test_curses.TestAscii.test_ctrl) ... ok
                                                                                                                                      test_ctypes (test.test_curses.TestAscii.test_ctypes) ... ok
                        test_unctrl (test.test_curses.TestAscii.test_unctrl) ... ok
                                                                                   TERM=xterm-256color
                                                                                                      test_attributes (test.test_curses.TestCurses.test_attributes) ... ok
 test_background (test.test_curses.TestCurses.test_background) ... ok
                                                                     test_beep (test.test_curses.TestCurses.test_beep) ... ok
                                                                                                                             test_borders_and_lines (test.test_curses.TestCurses.test_borders_and_lines) ... ok
                                      test_chgat (test.test_curses.TestCurses.test_chgat) ... ok
                                                                                                test_clear (test.test_curses.TestCurses.test_clear) ... ok
                                                                                                                                                          test_color_attrs (test.test_curses.TestCurses.test_color_attrs) ... ok
                                                       test_color_content (test.test_curses.TestCurses.test_color_content) ... ok
                                                                                                                                 test_create_windows (test.test_curses.TestCurses.test_create_windows) ...

                                 == Tests result: FAILURE ==

                                                            1 test failed:
                                                                              test_curses

                                                                                         Total duration: 198 ms
                                                                                                               Total tests: run=0
                                                                                                                                 Total test files: run=1/1 failed=1
                                                                                                                                                                   Result: FAILURE
         %

I'm not a curses expert, but I'm not sure that this is something we can fix on our end, so this should probably be closed. It might be worth disabling the crashy tests if we can detect the broken curses version, though?

@brandtbucher
Copy link
Member

brandtbucher commented Sep 20, 2023

CC @Yhg1s as a curses expert.

@brandtbucher
Copy link
Member

The affected tests appear to be test_create_windows, test_move_cursor, test_output_character, test_refresh, and test_refresh_control.

@ned-deily
Copy link
Member

FWIW this problem should not arise when using a python from a macOS python.org installer or MacPorts as they supply their own version of ncurses and do not use the system version.

@cdwrobg
Copy link

cdwrobg commented Sep 20, 2023

FWIW this problem should not arise when using a python from a macOS python.org installer or MacPorts as they supply their own version of ncurses and do not use the system version.

I can confirm this. I was only able to reproduce it for @timway when using the system version of python.

@brandtbucher
Copy link
Member

Downgrading from Xcode 15.0 (curses.ncurses_version(major=6, minor=0, patch=20150808)) to 14.3 (curses.ncurses_version(major=5, minor=7, patch=20081102)) fixes it for me.

@gpshead gpshead changed the title Refresh Screen Provided By curses.wrapper Causes Seg Fault (MacOS) Refresh Screen Provided By curses.wrapper Causes Seg Fault (macOS, xcode 15 Apple supplied Python build) Sep 20, 2023
@gpshead gpshead changed the title Refresh Screen Provided By curses.wrapper Causes Seg Fault (macOS, xcode 15 Apple supplied Python build) Refresh Screen Provided By curses.wrapper Causes Seg Fault (macOS, xcode 15 Apple supplied ncurses 6.0 breakage) Sep 20, 2023
@gpshead
Copy link
Member

gpshead commented Sep 20, 2023

It sounds like they shipped a potentially broken version of ncurses embedded within xcode 15?

We've been building and linking with ncurses >= 6 in the rest of the world since 2015 without this problem...

@gpshead
Copy link
Member

gpshead commented Sep 20, 2023

It is really strange behavior on Apple's part to "upgrade" to providing 6.0 from 2015 in their 2023 toolchain when ncurses 6.4 came out last year.

We're at the point where it'd be fine for CPython to simply drop the ability to build and link against 6.0 in 3.13+ given that no supported Linux distro platform will be using it anymore by the time we release 3.13. (RHEL 8 shipped with ncurses 6.1 making that a reasonable minimum) -- I do not expect or encourage anyone to go through and do that to the _curses module code.

Recommendation: do what we do with python.org builds, never build against apple's ncurses.

If a workaround for this isn't obvious to someone with a macos debugger, it is reasonable to have a configure check blocklist the xcode 15 supplied ncurses and refuse to use it. Spending our time dealing with brazenly outdated libraries embedded in someone elses toolchain that we don't ship our own builds with isn't worthwhile (it'd never end).

@timway
Copy link
Author

timway commented Sep 20, 2023

Thank you all for looking into this - as next steps I've found the Feedback Assistant and the general feedback form on Apple's website. I'll work to post to those methods and provide any feedback here.

I could also look at writing a small C example and see if I can confirm if the problem is in their ncurses implementation and fully removed from Python or not.

@vstinner
Copy link
Member

If you want to require a specific version or recommend libedit or ncurses on macOS, you can update the doc: https://docs.python.org/dev/using/configure.html

@ronaldoussoren
Copy link
Contributor

Issues like this should also be reported to Apple as this is a crash with their copy of Python.

I've filed FB13196764 about this.

@gpshead
Copy link
Member

gpshead commented Sep 22, 2023

Thanks Ronald!

@ronaldoussoren
Copy link
Contributor

FWIW this problem should not arise when using a python from a macOS python.org installer or MacPorts as they supply their own version of ncurses and do not use the system version.

Our copy of ncurses is ancient though and should be updated to a more recent version. But that's something for a different issue, and needs careful testing because we rely on the system terminfo database.

aphedges added a commit to aphedges/pyenv that referenced this issue Oct 5, 2023
XCode Command Line Tools 15.0 was recently released, and it contains a
broken version of ncurses 6.0. Some uses of Python's `curses` module
will segfault when compiled with it. The solution is to switch to using
the version of ncurses from Homebrew, which is currently 6.4. Support
for ncurses 6 was added to Python 3.7 and was backported to 3.6 and 2.7,
so this change should not break any recently supported Python versions.

See python/cpython#109617 and
python/cpython#69906 for more information.
aphedges added a commit to aphedges/pyenv that referenced this issue Oct 5, 2023
XCode Command Line Tools 15.0 was recently released, and it contains a
broken version of ncurses 6.0. Some uses of Python's `curses` module
will segfault when compiled with it. The solution is to switch to using
the version of ncurses from Homebrew, which is currently 6.4. Support
for ncurses 6 was added to Python 3.7 and was backported to 3.6 and 2.7,
so this change should not break any recently supported Python versions.

I tested this commit with Python 3.12, 3.11, and 2.7, and all tests in
the `test.test_curses` module passed without issue.

See python/cpython#109617 and
python/cpython#69906 for more information.
native-api pushed a commit to pyenv/pyenv that referenced this issue Oct 6, 2023
XCode Command Line Tools 15.0 was recently released, and it contains a
broken version of ncurses 6.0. Some uses of Python's `curses` module
will segfault when compiled with it. The solution is to switch to using
the version of ncurses from Homebrew, which is currently 6.4. Support
for ncurses 6 was added to Python 3.7 and was backported to 3.6 and 2.7,
so this change should not break any recently supported Python versions.

Tested with Python 3.12, 3.11, and 2.7, and all tests in
the `test.test_curses` module pass without issue.

See python/cpython#109617 and
python/cpython#69906 for more information.
@sorcio
Copy link
Contributor

sorcio commented Oct 9, 2023

The root issue is that is_pad is weakly linked and is only available in macOS 14 (and iOS 17 etc etc), but it's exposed in recent SDKs. This confuses the configure check for HAVE_CURSES_IS_PAD which will see the function, although it may be unavailable at runtime.

The compiler warns about this with a warning: 'is_pad' is only available on macOS 14.0 or newer [-Wunguarded-availability-new].


There is a weird interaction of HAVE_CURSES_IS_PAD, WINDOW_HAS_FLAGS, and NCURSES_OPAQUE because of bpo-25720. My head hurts following all the cases. But I'm pretty sure that there is no need to do a runtime check, because if you are in one of the cases that would need it (Apple, recent SDK) you also have at the very least ncurses 5.7, so it should always be safe to use the non-opaque fallback.

@sorcio
Copy link
Contributor

sorcio commented Oct 9, 2023

Ugh, just realized the implication that NCURSES_VERSION_MAJOR/MINOR/PATCH don't correspond to the actual libncurses version.

@vstinner
Copy link
Member

Maybe the magic macOS macro __builtin_available(macOS ...) should be used for ncurses.

@sorcio
Copy link
Contributor

sorcio commented Oct 11, 2023

Maybe the magic macOS macro __builtin_available(macOS ...) should be used for ncurses.

It shouldn't be necessary. It's a funny decision on Apple's side to use availability attributes on a function like is_pad. How do you even implement a fallback if the function is not available at runtime?

For ncurses, it should be enough to default to NCURSES_OPAQUE=0 which:

  • in ncurses.h versions since the introduction of is_pad (20090906): also exposes is_pad as a macro, so we never touch the function, and we don't have to worry about runtime checks
  • in ncurses.h prior to that:
    • since 20070303: makes win->_flags inspectable so the current fallback still works
    • in older versions: is ignored, the struct is always transparent
  • in other curses: hopefully does nothing, and the current detection logic still works

(Hope I got this right. It's a bit of a mess to track so I needed to write this down)

If NCURSES_OPAQUE=0 is ever removed from ncurses, and a future Python version will support that release, it will hopefully be far enough in the future that nobody will be concerned about running that Python on macOS < 14 (hello person from the far future who found this bug, please don't be angry at me).

That said, I don't understand why #4164 decided to opt out of setting NCURSES_OPAQUE=0 in case the function is available. I see little reason to switch working code to use the opaque functions (unless it needs to use ncurses from parallel threads).

@sorcio
Copy link
Contributor

sorcio commented Oct 11, 2023

This is what I had in mind main...sorcio:cpython:curses-mac-fix

@vstinner
Copy link
Member

As a fan of Python limited C API, I would prefer that Python does not use NCURSES_OPAQUE=0. While it works today, it may break tomorrow.

Another way should be found to fix the Python module.

"is_pad: returns TRUE if the window is a pad i.e., created by newpad"

If the function is not available, can we just not make it available in Python?

@sorcio
Copy link
Contributor

sorcio commented Oct 12, 2023

@vstinner is_pad is not exposed as a function in the Python module. It's used internally in just a few methods (e.g. window.refresh() or window.subwin()) to transparently work on either windows or pads.

Historically no [n]curses implementations exposed it as a function until ncurses 5.7.20090906. When building against an older version of the headers (such as the one included in Apple SDKs prior to Xcode 15), _cursesmodule.c would use the alternative technique that has been supported for 25 or so years: checking for win->_flags & _ISPAD. Note that with my change _cursesmodule.c would use the is_pad macro if [n]curses.h defines it, rather than relying on internals.

While it works today, it may break tomorrow.

Definitely. If ncurses ever removes the non-opaque option, it would break setups where CPython links system libncurses dynamically and an older library is provided (such as the 5.7 one on macOS < 14). So, yeah, some hypothetical future CPython built in some specific way depending on some hypothetical future ncurses would probably not be able to import system ncurses on macOS 13.6. But I don't think that's an issue!

As a fan of Python limited C API

Uh, does the limited C API make promises about curses? Technically py_curses.h is not an "internal" header, and a user might import it, and defining NCURSES_OPAQUE might get in their way. But this is completely undocumented use, and it's already platform-dependent.

@ronaldoussoren
Copy link
Contributor

ronaldoussoren commented Oct 12, 2023 via email

@sorcio
Copy link
Contributor

sorcio commented Oct 12, 2023

I don't understand what "instead of disabling is_pad" means.

@ronaldoussoren
Copy link
Contributor

Sorry about the slow response, I was traveling at the time.

I don't understand what "instead of disabling is_pad" means.

You wrote:

The root issue is that is_pad is weakly linked and is only available in macOS 14 (and iOS 17 etc etc), but it's exposed in recent SDKs. This confuses the configure check for HAVE_CURSES_IS_PAD which will see the function, although it may be unavailable at runtime.

The compiler warns about this with a warning: 'is_pad' is only available on macOS 14.0 or newer [-Wunguarded-availability-new].

The IMHO correct way to deal with this is to use is_pad (and other curses functions new in 14.0) only when running on 14.0 or later by using __builtin_available. This does complicate the code in Modules/_cursesmodule.c a little, but the amount of change likely can be limited by slightly restructuring the code (such as making py_is_pad a static inline function instead of a macro). We already do this in a number of other modules, including some much more involved changes in posixmodule.c.

@sorcio
Copy link
Contributor

sorcio commented Oct 19, 2023

Thanks for the clarification @ronaldoussoren. Hope the sprints went great!

The issue with using __builtin_available in this specific case is that, at runtime, you don't have a fallback available. Let's say you have something like:

#define HAVE_CURSES_IS_PAD_RUNTIME __builtin_available(macOS 14.0, *)
// ...
if (HAVE_CURSES_IS_PAD_RUNTIME) {
    return is_pad(w);
} else {
    // ... what goes here? ...
}

If you have NCURSES_OPAQUE=1 (the default in newer ncurses.h), the true branch can use the is_pad() function, but the false branch doesn't have a way to check if the window is a pad. The check is not optional (i.e. you can't always return false), it's necessary in some code paths for correctness.

You may set NCURSES_OPAQUE=0, so the false branch can work (either by using the is_pad macro exported by ncurses.h, or by inspecting w->_flags). But in this case the availability check is unnecessary, because ncurses.h doesn't even expose the is_pad function symbol (only the macro), and we have nothing to check, because it's always available.

That's why in main...sorcio:cpython:curses-mac-fix I defaulted to NCURSES_OPAQUE=0 and haven't used the runtime availability check.

An alternative might be to have a separate translation unit, let's say a curses_compat.c, which is (conditionally?) compiled with NCURSES_OPAQUE=0, and leave everything else to the ncurses default. The compatibility layer would expose py_is_pad. It would maybe be cleaner, but I didn't want to go this way because it felt overkill for just a single function.

Since I spent a minute researching this, I'm happy to open a PR.

@vstinner
Copy link
Member

if (HAVE_CURSES_IS_PAD_RUNTIME) {
    return is_pad(w);
} else {
    // ... what goes here? ...
}

Don't define is_pad macro if the function is not available. Modules/_cursesmodule.c can already be built without is_pad() function. Example in _curses_window_echochar_impl():

#ifdef py_is_pad                        
    if (py_is_pad(self->win)) {                                          
        return PyCursesCheckERR(pechochar(self->win, ch_ | (attr_t)attr),   
                                "echochar");
    }                                                                   
    else                                                       
#endif                                                                   
        return PyCursesCheckERR(wechochar(self->win, ch_ | (attr_t)attr),
                                "echochar");       

Or well, just always return 0?

@sorcio
Copy link
Contributor

sorcio commented Oct 20, 2023

Modules/_cursesmodule.c can already be built without is_pad() function

Eh, kinda, but not really in a way that solves the problem. It falls back in two different ways, depending on checks done at configure time:

  1. if is_pad is a function, define py_is_pad to use that;
  2. otherwise: if is_pad is not available1, but window flags are inspectable, it defines py_is_pad to check flags. This is what happens on older Apple SDKs (which work!) and on any system with ncurses < 5.8.
  3. otherwise: don't define py_is_pad at all, and ignore the distinction between windows and pads. This is meant to support some other curses, and is never the case with ncurses.

What you are suggesting is 3, but ncurses doesn't like that, e.g. curs_pad(3x):

   It  is  not  legal  to  call  wrefresh  with  a pad as an argument; the
   routines prefresh or pnoutrefresh should be called instead. 

In the example you pasted, this would end up calling wechochar on a pad instead of pechochar, which might not work as expected, or impact performance. Note that this might break in a way that is not covered by test_curses. I don't see tests on pads stuff.


On a personal note, I'm getting confused by this conversation. I suggested a fix and tested it. It took some work to figure out the different cases, and I documented that work in this issue. It's not really complicated, only a bit messy with all the historical cruft. But the comments don't address the proposed solution, and seem to argue that some other unspecified solution should be preferable. I have a bucketful of impostor syndrome to handle, and I'd prefer not to feed it more.

Footnotes

  1. yeah, in the current setup, with ncurses, either is_pad is a function (detected by configure, which would set HAVE_CURSES_IS_PAD) or it's not used at all. There is no path that uses the is_pad macro from ncurses.h, which is legal and documented.

@vstinner
Copy link
Member

I understand that with Xcode 15.0, the is_pad() function is not available, but win->_flags & _ISPAD can be tested if the NCURSES_OPAQUE macro is set to 0. For me, it's surprising to have to tune NCURSES_OPAQUE macro and to inspect a structure member starting with an underscore.

But apparently, this issue affects many macOS issues, it's annoying, and I should stop nitpicking. So just use NCURSES_OPAQUE=0 on macOS, and it should "just works", no?

Does someone want to propose a PR? Nobody proposed a PR so far, no?

@sorcio
Copy link
Contributor

sorcio commented Oct 24, 2023

I understand that with Xcode 15.0, the is_pad() function is not available, but win->_flags & _ISPAD can be tested if the NCURSES_OPAQUE macro is set to 0. For me, it's surprising to have to tune NCURSES_OPAQUE macro and to inspect a structure member starting with an underscore.

Nope. "Inspecting a structure member starting with an underscore" is what cursesmodule.c does now and has been doing for the last couple decades. If you built on Mac before Xcode 15, you used that.

There is a convenient and documented is_pad macro, defined in Ncurses public headers for a long while now. Since Apple now includes this version, we can use it.

Does someone want to propose a PR? Nobody proposed a PR so far, no?

I asked for early feedback on a diff a couple times above. I will open a PR in a little bit. Maybe it will reduce the confusion.

@sorcio
Copy link
Contributor

sorcio commented Oct 29, 2023

Suggestion: let's rename the issue to something that better describes the problem.

@efalk
Copy link

efalk commented Jan 2, 2024

Is there a workaround? I want to get back to work on my project.

@timway
Copy link
Author

timway commented Jan 2, 2024

Is there a workaround? I want to get back to work on my project.

I'm no longer able to replicate the issue with Sonoma 14.2.1 and Command Line Tools for Xcode 15.1.

Alternatively, versions from Python.org shouldn't have the issue.

Edit: It looks like 14.2 got some patches for ncurses behavior (https://support.apple.com/en-us/HT214036). While they don't specifically mention this as an issue it does appear to resolve it for me at least.

@efalk
Copy link

efalk commented Jan 2, 2024

Yes, downloading the latest directly from python.org made the problem go away. It would be nice if there were something I could do in code, other than telling customers "you need to install a new version of Python on your system"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OS-mac type-crash A hard crash of the interpreter, possibly with a core dump
Projects
Status: No status
Development

No branches or pull requests

10 participants