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

On FreeBSD, signal.NSIG is smaller than biggest signal value #64783

Closed
jgehrcke mannequin opened this issue Feb 10, 2014 · 15 comments
Closed

On FreeBSD, signal.NSIG is smaller than biggest signal value #64783

jgehrcke mannequin opened this issue Feb 10, 2014 · 15 comments
Labels
OS-freebsd stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@jgehrcke
Copy link
Mannequin

jgehrcke mannequin commented Feb 10, 2014

BPO 20584
Nosy @pitrou, @vstinner, @osvenskan, @jgehrcke
Files
  • signal_nsig_freebsd.patch
  • signal_nsig_freebsd-2.patch
  • 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 = None
    created_at = <Date 2014-02-10.17:36:38.170>
    labels = ['type-bug', 'library']
    title = 'On FreeBSD, signal.NSIG is smaller than biggest signal value'
    updated_at = <Date 2014-10-25.16:16:19.959>
    user = 'https://github.com/jgehrcke'

    bugs.python.org fields:

    activity = <Date 2014-10-25.16:16:19.959>
    actor = 'osvenskan'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['Library (Lib)']
    creation = <Date 2014-02-10.17:36:38.170>
    creator = 'jgehrcke'
    dependencies = []
    files = ['35310', '35317']
    hgrepos = []
    issue_num = 20584
    keywords = ['patch']
    message_count = 10.0
    messages = ['210854', '210892', '218895', '218896', '218913', '218914', '218916', '218954', '218958', '218984']
    nosy_count = 6.0
    nosy_names = ['pitrou', 'vstinner', 'osvenskan', 'jgehrcke', 'neologix', 'sdaoden']
    pr_nums = []
    priority = 'normal'
    resolution = None
    stage = None
    status = 'open'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue20584'
    versions = ['Python 2.7', 'Python 3.4', 'Python 3.5']

    @jgehrcke
    Copy link
    Mannequin Author

    jgehrcke mannequin commented Feb 10, 2014

    On FreeBSD, signal.NSIG is smaller than what the documentation promises: "One more than the number of the highest signal number".

    On Linux, the highest numerical signal value is smaller/equal signal.NSIG (expected behavior):

    >>> import signal
    >>> signals = [s for s in dir(signal) if s.startswith("SIG")]
    >>> max([(getattr(signal, s), s) for s in signals])
    (64, 'SIGRTMAX')
    >>> signal.NSIG
    65

    On FreeBSD (since version 7, when SIGRTMIN/MAX have been introduced), Python's signal.NSIG is either 32 (if defined by the system, depending on __BSD_VISIBLE, see http://svnweb.freebsd.org/base/head/sys/sys/signal.h?revision=233519&view=markup#l331) or 64 (if chosen by signalmodule.c as a fallback). In any case, on FreeBSD the numerical values of SIGRTMIN/MAX are 65 and 126 and therefore both greater than Python's signal.NSIG:
    http://svnweb.freebsd.org/base/head/sys/sys/signal.h?revision=233519&view=markup#l117

    Consequently, Python's signal module exposes a number NSIG which is not 'true'. Two disadvantages:

    • signal.NSIG is just not meaningful on FreeBSD. It is part of the signal module's public interface, and should do what its documentation says: "One more than the number of the highest signal number".

    • this might lead to unexpected behavior when for instance calling signal.signal(signal.SIGRTMAX, signal.SIG_DFL). This works on Linux, but fails with a ValueError on FreeBSD: raised directly by signalmodule.c, because sig_num >= NSIG, i.e. sig_num seemingly is an invalid signal number, although it is not (https://github.com/python/cpython/blob/3.3/Modules/signalmodule.c#L323). This is the reason why I became aware of this topic.

    I see three arguments here:

    • if the system does not provide NSIG via signal.h and Python's signalvalue makes the wrong guess (i.e. fallback to 64), then signalmodule.c would be to blame.

    • if the system provides NSIG via signal.h and this is not the true maximum, then one could say that Python is not to blame.

    • on the other hand, signalmodule.c is aware of all signals that it actively checked for and therefore could derive "One more than the number of the highest signal number" on its own via something in the lines of max(signal_values)+1.

    Regarding the latter point: if Python misses to check for a valid signal on a certain platform, then this actively derived NSIG value would not be entirely correct, either, seen from the platform's perspective. But the signal module would then at least be consistent with itself.

    In case of FreeBSD, I am actually not sure if signal.NSIG *is* provided by the system or determined by the fallback method in signalmodule.c (I can't get my hands on a FreeBSD machine at the moment).

    What do you think?

    Btw, parts of this have already been mentioned here: http://bugs.python.org/issue12060

    @jgehrcke jgehrcke mannequin added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Feb 10, 2014
    @jgehrcke
    Copy link
    Mannequin Author

    jgehrcke mannequin commented Feb 11, 2014

    As a follow-up, relevant output from FreeBSD 9:

    $ python
    Python 2.7.5 (default, Dec 20 2013, 21:12:37)
    [GCC 4.2.1 20070831 patched [FreeBSD]] on freebsd9
    Type "help", "copyright", "credits" or "license" for more information.
    >>> import signal
    >>> signals = [s for s in dir(signal) if s.startswith("SIG")]
    >>> max((getattr(signal, s), s) for s in signals)
    (126, 'SIGRTMAX')
    >>> signal.NSIG
    32
    >>> signal.signal(signal.SIGRTMAX, lambda *a: None)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    ValueError: signal number out of range

    Hence, it's not the fallback to 64, it's FreeBSD's signal.h telling that NSIG is 32.

    @jgehrcke
    Copy link
    Mannequin Author

    jgehrcke mannequin commented May 22, 2014

    If you are thinking TL;DR:

    This fails on FreeBSD:

    >>> signal.signal(signal.SIGRTMAX, lambda *a: None)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    ValueError: signal number out of range

    Although of infrequent use, I doubt that this is expected or desired behavior.

    @vstinner
    Copy link
    Member

    The current implementation of _signal requires a limit on the number of signals to its internal array used to store Python callback:

    static volatile struct {
        sig_atomic_t tripped;
        PyObject *func;
    } Handlers[NSIG];

    If you want to kill the arbitrary limit, you need to change this structure.

    Maybe we need to find NSIG value differently on FreeBSD? For example try to use _SIG_MAXSIG.
    http://lists.freebsd.org/pipermail/freebsd-doc/2010-August/017500.html

    Please try attached on FreeBSD.

    @pitrou
    Copy link
    Member

    pitrou commented May 22, 2014

    If you want to kill the arbitrary limit, you need to change this
    structure.

    Or the structure could simply host up to 256 handlers, regardless of NSIG.
    I'm uncomfortable with tweaking NSIG specifically for FreeBSD. If the FreeBSD headers export the wrong value, it's not really Python's problem.

    @neologix
    Copy link
    Mannequin

    neologix mannequin commented May 22, 2014

    Or the structure could simply host up to 256 handlers, regardless of NSIG.
    I'm uncomfortable with tweaking NSIG specifically for FreeBSD. If the FreeBSD headers export the wrong value, it's not really Python's problem.

    Agreed.
    And the current code is already complicated enough:

    #ifndef NSIG
    # if defined(_NSIG)
    #  define NSIG _NSIG            /* For BSD/SysV */
    # elif defined(_SIGMAX)
    #  define NSIG (_SIGMAX + 1)    /* For QNX */
    # elif defined(SIGMAX)
    #  define NSIG (SIGMAX + 1)     /* For djgpp */
    # else
    #  define NSIG 64               /* Use a reasonable default value */
    # endif
    #endif

    @vstinner
    Copy link
    Member

    vstinner commented May 22, 2014

    Extract of system signal.h:

    #if __BSD_VISIBLE
    #define NSIG            32      /* number of old signals (counting 0) */
    #endif

    whereas <sys/_sigset.h> contains:

    #define _SIG_MAXSIG     128

    In signalmodule.c, NSIG is still important in the function sigset_to_set(): we need to have the exact maximum signal number of a sigset.

    I prefer to make signalmodule.c a little big uglier to fix the NSIG value. I tested attached signal_nsig_freebsd-2.patch on FreeBSD 9.

    I suggest to backport this fix to Python 2.7 and 3.4.

    @jgehrcke
    Copy link
    Mannequin Author

    jgehrcke mannequin commented May 23, 2014

    We should match the unit test with the documentation for signal.NSIG. Either the code or the docs or both need to change.

    Currently the docs say that signal.NSIG is "One more than the number of the highest signal number." ("https://docs.python.org/3.4/library/signal.html#signal.NSIG).

    In case of FreeBSD's _SIG_MAXSIG (128) the documentation is still wrong: the highest signal value MAX is 126 (see http://bugs.python.org/issue20584#msg210892). According to the docs, NSIG should then be 127.

    In signal_nsig_freebsd-2.patch the test self.assertLess(max(signals), signal.NSIG) tests for NSIG > MAX instead of for NSIG = MAX+1.

    So, either

    • we count signals by ourselves and build our own value of NSIG, in which case we can guarantee that NSIG = MAX+1 or

    • we rely on the platform header files for extracting NSIG, but must note in the docs that NSIG is not always MAX+1.

    The current patch + a documentation change would implement the latter case.

    What is the exact meaning of _SIG_MAXSIG, where is that meaning defined?

    @sdaoden
    Copy link
    Mannequin

    sdaoden mannequin commented May 23, 2014

    Salut!, amis français! (Und auch sonst so, natürlich.)

    POSIX has recently standardized a NSIG_MAX constant in <limits.h> [1]:

    The value of {NSIG_MAX} shall be no greater than the number of signals that the sigset_t type (see [cross-ref to <signal.h>]) is capable of representing, ignoring any restrictions imposed by sigfillset() or sigaddset().

    I'm personally following an advise of Rich Felker in the meantime:

      #ifdef NSIG_MAX
      # undef NSIG
      # define NSIG           NSIG_MAX
      #elif !defined NSIG
      # define NSIG           ((sizeof(sigset_t) * 8) - 1)
      #endif

    That is for "old" signals only, there; maybe reducing this to

      #undef NSIG
      #ifdef NSIG_MAX
      # define NSIG  NSIG_MAX
      #else
      # define NSIG  ((sizeof(sigset_t) * 8) - 1)
      #endif 

    should do the trick for Python on any POSIX system?
    Ciao from, eh, gray, Germany :)

    [1] <http://austingroupbugs.net/view.php?id=741#c1834\>

    @neologix
    Copy link
    Mannequin

    neologix mannequin commented May 23, 2014

    Jan-Philip Gehrcke added the comment:

    Currently the docs say that signal.NSIG is "One more than the number of the highest signal number." ("https://docs.python.org/3.4/library/signal.html#signal.NSIG).

    In case of FreeBSD's _SIG_MAXSIG (128) the documentation is still wrong: the highest signal value MAX is 126 (see http://bugs.python.org/issue20584#msg210892). According to the docs, NSIG should then be 127.

    Yeah, but it doesn't matter.
    We shouldn't be exposing this constant in the first place, all that
    matters is that we accept all valid signals, and we don't crash whe
    passing an invalid once (ssee below).

    > Steffen Daode Nurpmeso added the comment:
    >
    >   #ifdef NSIG_MAX
    >   # undef NSIG
    >   # define NSIG           NSIG_MAX
    >   #elif !defined NSIG
    >   # define NSIG           ((sizeof(sigset_t) * 8) - 1)
    >   #endif
    >
    > should do the trick for Python on any POSIX system?
    

    This assumes that sigset_t is implemented as a raw bitmap, which isn't
    documented (is could be implemented by an arbitrary data structure).
    On the other hand, it's really really likely, and should guarantee
    that sigset & Co don't crash on too large values (or write to
    arbitrary memory locations like fd_set when passed fd > FD_SETSIZE).

    So I think we should go for the above patch (Steffen's), with a doc
    update saying that "NSIG is a value larger than the largest signals".
    If the OS headers don't provide it, it's not our business to try to
    infer it.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @Jehops
    Copy link

    Jehops commented Apr 20, 2022

    Just an update to say that the mismatch on FreeBSD still exists.

    % uname -sbr
    FreeBSD 14.0-CURRENT ca7ddedc9f1a415a5fc4bf1eed3b9faeb6fb0c71
    % python3.8
    Python 3.8.13 (default, Mar 24 2022, 17:57:22)
    [Clang 13.0.0 (git@github.com:llvm/llvm-project.git llvmorg-13.0.0-0-gd7b669b3a on freebsd14
    Type "help", "copyright", "credits" or "license" for more information.
    >>> import signal
    >>> signals = [s for s in dir(signal) if s.startswith("SIG")]
    >>> max([(getattr(signal, s), s) for s in signals])
    (<Signals.SIGRTMAX: 126>, 'SIGRTMAX')
    >>> signal.NSIG
    32
    

    @vstinner
    Copy link
    Member

    #91145 seems to be a duplicate.

    @vstinner
    Copy link
    Member

    I wrote #91929 to fix this issue and update the NSIG documentation.

    @vstinner
    Copy link
    Member

    I marked #91145 as a duplicate of this issue.

    vstinner added a commit that referenced this issue Apr 25, 2022
    Fix signal.NSIG value on FreeBSD to accept signal numbers greater
    than 32, like signal.SIGRTMIN and signal.SIGRTMAX.
    
    * Add Py_NSIG constant.
    * Add pycore_signal.h internal header file.
    * _Py_Sigset_Converter() now includes the range of valid signals in
      the error message.
    tiran added a commit to tiran/cpython that referenced this issue May 5, 2022
    @vstinner
    Copy link
    Member

    Fixed by commit 20cc695 of #91929.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    OS-freebsd stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants