ossaudiodev's writeall method doesn't check that the FD is less than FD_SETSIZE when passing it to FD_SET: since FD_SET typically doesn't do bound check, it will write to a random location in memory (in this case on the stack).
I've attached a test that triggers a segfault on my 32-bit Linux box:
- you must have an OSS-compatible device as /dev/dsp (if you don't you can use "modprobe snd_pcm_oss")
- it tries to increase RLIMIT_NOFILE since it's usually defined to be the same as FD_SETSIZE (1024 on Linux). The script must be run as root for that.
A patch is attached.
The only other place where I've seen a similar problem is in Module/readline.c: I'm not sure it's worth adding this check there :-)

> ossaudiodev's writeall method doesn't check that the FD is less than
> FD_SETSIZE when passing it to FD_SET: since FD_SET typically doesn't
> do bound check, it will write to a random location in memory (in this
> case on the stack).
> I've attached a test that triggers a segfault on my 32-bit Linux box:
> - you must have an OSS-compatible device as /dev/dsp (if you don't you
> can use "modprobe snd_pcm_oss")
> - it tries to increase RLIMIT_NOFILE since it's usually defined to be
> the same as FD_SETSIZE (1024 on Linux). The script must be run as root
> for that.
> A patch is attached.

Well, the test doesn't work here ("IOError: [Errno 16] Device or
resource busy: '/dev/dsp'", probably because of PulseAudio already using
it), but the patch looks simple enough.

By the way, this function still uses "y#" instead of "y*", this could be
the topic of another issue if you are interested.

You don't check that 0 <= fd (e.g. oss.close()).

The select has a specific code for Visual Studio (don't check v < FD_SETSIZE):

#if defined(_MSC_VER)
        max = 0;                             /* not used for Win32 */
#else  /* !_MSC_VER */
        if (v < 0 || v >= FD_SETSIZE) {
            PyErr_SetString(PyExc_ValueError,
                        "filedescriptor out of range in select()");
            goto finally;
        }
        if (v > max)
            max = v;
#endif /* _MSC_VER */

Python has a _PyVerify_fd() function. We might write a similar function/macro to check if a file descriptor can be used in a file descriptor set. FD_SET() is used in the oss, readline, socket and _ssl modules. The socket module has a IS_SELECTABLE() macro:

#ifdef Py_SOCKET_FD_CAN_BE_GE_FD_SETSIZE
/* Platform can select file descriptors beyond FD_SETSIZE */
#define IS_SELECTABLE(s) 1
#elif defined(HAVE_POLL)
/* Instead of select(), we'll use poll() since poll() works on any fd. */
#define IS_SELECTABLE(s) 1
/* Can we call select() with this socket without a buffer overrun? */
#else
/* POSIX says selecting file descriptors beyond FD_SETSIZE
   has undefined behaviour.  If there's no timeout left, we don't have to
   call select, so it's a safe, little white lie. */
#define IS_SELECTABLE(s) ((s)->sock_fd < FD_SETSIZE || s->sock_timeout <= 0.0)
#endif

Note: do you really use the OSS module? On which OS? :)

> You don't check that 0 <= fd (e.g. oss.close()).
>

Actually, none of ossaudiodev's method does...

This makes it even easier to trigger a segfault (at least on RHEL4):
"""
import ossaudiodev

o = ossaudiodev.open('/dev/dsp', 'w')
o.close()
o.writeall(b'toto')
"""

I've attached a patch to fix that (check that the underlying FD isn't closed).

> The select has a specific code for Visual Studio (don't check v < FD_SETSIZE):

> Python has a _PyVerify_fd() function. We might write a similar function/macro to check if a file descriptor can be used in a file descriptor set. FD_SET() is used in the oss, readline, socket and _ssl modules. The socket module has a IS_SELECTABLE() macro:

So, this _PyCheckSelectable_fd ? function/macro would:
- return true (1) on Visual Studio or if
Py_SOCKET_FD_CAN_BE_GE_FD_SETSIZE is defined
- return false (0) if the file descriptor is greater than FD_SETSIZE otherwise
Do we agree on that?
Where should I add it? selectmodule, posixmodule, somewhere else?

>
> Note: do you really use the OSS module? On which OS? :)
>

Well, while we don't use ossaudiodev, we have a couple hundred Linux
machines at work, and we use OSS on Linux 2.6.9 kernels (and Python
2.3.4 ;-) )

> So, this _PyCheckSelectable_fd ? function/macro would:
> - return true (1) on Visual Studio or if
> Py_SOCKET_FD_CAN_BE_GE_FD_SETSIZE is defined
> - return false (0) if the file descriptor is greater than FD_SETSIZE otherwise
> Do we agree on that?

Py_SOCKET_FD_CAN_BE_GE_FD_SETSIZE is specific to Windows.

/* WinSock does not use a bitmask in select, and uses
   socket handles greater than FD_SETSIZE */
#define Py_SOCKET_FD_CAN_BE_GE_FD_SETSIZE

I don't understand if socket file descriptors are different than
(classic) file descriptors.

socketmodule.c uses Py_SAFE_DOWNCAST(s->sock_fd+1, SOCKET_T, int), which
means that the socket type can be different (bigger) than int.

"#if defined(_MSC_VER)" is maybe redundant with
Py_SOCKET_FD_CAN_BE_GE_FD_SETSIZE (with "#ifdef MS_WINDOWS" ?).

Does other compilers, like Cygwin / MinGW, also use WinSock?

> Where should I add it? selectmodule, posixmodule, somewhere else?

_Py_Verify_fd() is defined in posixmodule.c and fileobject.h.

If you would like write a _PyCheckSelectable_fd() function/macro, it can
be defined in fileobject.h, and implemented anymore. selectmodule.c is
maybe a better choice than posixmodule.c because posixmodule.c doesn't
use select. Or maybe in socketmodule.c if you reuse IS_SELECTABLE code.

For the name, I would prefer _PyIsSelectable_fd(). With "check", I would
have to read the name to check if it should return 0 or non-zero if the
fd is selectable.

--

Instead of _PyCheckSelectable_fd(), we can maybe do even better: write a
function to check if a file descriptor is ready to read or write using
poll() (of select() if poll() is not available). Example:

int _Py_FDIsReady(int fd, int writing, double timeout);

Returns:
1: fd is ready to read, or to write if writing is set
0: fd is not ready
-1: error, check errno (or maybe raise a Python error?
internal_select_ex() in socketmodule.c doesn't raise an exception)

("_Py_FDIsReady" name is horrible, but I don't have a better suggestion
yet)

poll() accepts negative timeout, whereas select() doesn't, and so
_PyCheckSelectable_fd() should raise an error if timeout is negative to
be portable.

I propose to use poll() rather than select() because I suppose that it a
little bit faster (maybe only if the fd number is big? e.g. fd=1023) The
difference to wait a single file descriptor is maybe nul.

What should be done in case of EINTR? PyThread_acquire_lock_timed() has
an intr_flag parameter to decide.

I don't think that "int fd" works with SOCKET_T (socket module), which
can be bigger than an int.

Well, _PyCheckSelectable_fd() is much more complex to implement than the
initial suggestion...

> I don't understand if socket file descriptors are different than
> (classic) file descriptors.

On Windows, sockets are completely independent from file descriptors.

A socket id can be large (typically over 1000), fortunately a fd_set is not indexed by descriptors; FD_SET just appends the socket descriptor to the array; there is a limit of 64 sockets, but no limit on the value of a descriptor.

Two patches attached:
- a patch checking that the ossaudiodev object isn't closed
- a patch adding _PyIsSelectable_fd()

> Two patches attached:
> - a patch checking that the ossaudiodev object isn't closed
> - a patch adding _PyIsSelectable_fd()

In oss_check_closed.diff, you might want to add tests for the other
methods as well. Otherwise, looks good!

Comments on is_selectable.diff.

> +#define IS_SELECTABLE(s) (_PyIsSelectable_fd((s)->sock_fd) || s->sock_timeout <= 0.0)

You may add parenthesis around the second s (even if it's unrelated to this issue and not a regression of your patch).

> +#ifdef Py_SOCKET_FD_CAN_BE_GE_FD_SETSIZE
> ...
>  #if defined(_MSC_VER)
>          max = 0;                 /* not used for Win32 */

I still don't understand these checks: why not testing simply for #ifdef MS_WINDOWS? MinGW or Cygwin have another implementation of select() which is more limited?

> In oss_check_closed.diff, you might want to add tests for the other
> methods as well.

I've added checks for other methods (not all of them, there are quite a few). I've also added a check for the mixer object.

> You may add parenthesis around the second s

Done.

> I still don't understand these checks

Me neither.
Since I don't know anything about Windows, I tried to keep the existing checks, but this doens't make much sense to me. Maybe Amaury can shed a light on this?

For oss_check_closed.diff, should I apply it to default only or to every branch?

> For oss_check_closed.diff, should I apply it to default only or to
> every branch?

Only default I'd say.

New changeset d0952a2fb7bd by Charles-François Natali in branch 'default':
Issue #12287: In ossaudiodev, check that the device isn't closed in several
http://hg.python.org/cpython/rev/d0952a2fb7bd

Brian, any comment about the Windows part (see Victor's message, http://bugs.python.org/issue12287#msg138137) ?

So, what should I do with is_selectable.diff?

> So, what should I do with is_selectable.diff?

It looks ok to me.

You might replace "#if defined(_MSC_VER)" with "#if defined(MS_WINDOWS)", but in another commit.

New changeset ff6adb867f40 by Charles-François Natali in branch '2.7':
Issue #12287: Fix a stack corruption in ossaudiodev module when the FD is
http://hg.python.org/cpython/rev/ff6adb867f40

The _socket module doesn't compile anymore on Windows:


Build started: Project: _socket, Configuration: Debug|Win32
Compiling...
socketmodule.c
29>..\Modules\socketmodule.c(1649) : warning C4013: '_PyIsSelectable_fd' undefined; assuming extern returning int
Linking...
   Creating library d:\cygwin\home\db3l\buildarea\2.7.bolen-windows\build\PCbuild\\_socket_d.lib and object d:\cygwin\home\db3l\buildarea\2.7.bolen-windows\build\PCbuild\\_socket_d.exp
socketmodule.obj : error LNK2019: unresolved external symbol __PyIsSelectable_fd referenced in function _sock_accept
d:\cygwin\home\db3l\buildarea\2.7.bolen-windows\build\PCbuild\\_socket_d.pyd : fatal error LNK1120: 1 unresolved externals
Build log was saved at "file://d:\cygwin\home\db3l\buildarea\2.7.bolen-windows\build\PCbuild\Win32-temp-Debug\_socket\BuildLog.htm"
_socket - 2 error(s), 1 warning(s)

> STINNER Victor <victor.stinner@haypocalc.com> added the comment:
>
> The _socket module doesn't compile anymore on Windows:
>

Fixed (that's why I wanted a Windows expert to have a look at this patch :-).

> You might replace "#if defined(_MSC_VER)" with "#if defined
> (MS_WINDOWS)", but in another commit.

I'd rather not modify code I don't understand. Plus, I have a really
poor Windows karma...

New changeset 852ca32eb18d by Charles-François Natali in branch '3.2':
Issue #12287: Fix a stack corruption in ossaudiodev module when the FD is
http://hg.python.org/cpython/rev/852ca32eb18d

New changeset ad1c09b6a5b9 by Charles-François Natali in branch 'default':
Issue #12287: Fix a stack corruption in ossaudiodev module when the FD is
http://hg.python.org/cpython/rev/ad1c09b6a5b9

Alright, committed to 2.7, 3.2 an default.
Seems to work on all the buildbots, closing.