classification
Title: ossaudiodev: stack corruption with FD >= FD_SETSIZE
Type: crash Stage: resolved
Components: Library (Lib) Versions: Python 3.1, Python 3.2, Python 3.3, Python 2.7, Python 2.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: amaury.forgeotdarc, brian.curtin, haypo, neologix, pitrou, python-dev
Priority: normal Keywords: needs review, patch

Created on 2011-06-08 20:37 by neologix, last changed 2011-08-28 17:22 by neologix. This issue is now closed.

Files
File name Uploaded Description Edit
test_oss.py neologix, 2011-06-08 20:37
is_selectable.diff neologix, 2011-06-10 22:17 review
oss_check_closed.diff neologix, 2011-06-10 22:17 review
Messages (22)
msg137923 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-06-08 20:37
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 :-)
msg137924 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-06-08 20:43
> 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.
msg137927 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2011-06-08 21:53
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? :)
msg138020 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-06-09 18:13
> 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 ;-) )
msg138031 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2011-06-09 22:53
> 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...
msg138075 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2011-06-10 11:47
> 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.
msg138135 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-06-10 21:45
Two patches attached:
- a patch checking that the ossaudiodev object isn't closed
- a patch adding _PyIsSelectable_fd()
msg138136 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-06-10 21:54
> 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!
msg138137 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2011-06-10 21:57
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?
msg138139 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-06-10 22:17
> 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?
msg138165 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-06-11 16:30
For oss_check_closed.diff, should I apply it to default only or to every branch?
msg138166 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-06-11 16:32
> For oss_check_closed.diff, should I apply it to default only or to
> every branch?

Only default I'd say.
msg138170 - (view) Author: Roundup Robot (python-dev) Date: 2011-06-11 16:58
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
msg140323 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-07-14 07:15
Brian, any comment about the Windows part (see Victor's message, http://bugs.python.org/issue12287#msg138137) ?
msg142904 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-08-24 19:40
So, what should I do with is_selectable.diff?
msg142906 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-08-24 20:02
> So, what should I do with is_selectable.diff?

It looks ok to me.
msg142947 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2011-08-24 23:07
You might replace "#if defined(_MSC_VER)" with "#if defined(MS_WINDOWS)", but in another commit.
msg143096 - (view) Author: Roundup Robot (python-dev) Date: 2011-08-28 14:21
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
msg143097 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2011-08-28 14:32
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)
msg143098 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-08-28 15:27
> 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...
msg143101 - (view) Author: Roundup Robot (python-dev) Date: 2011-08-28 16:09
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
msg143108 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-08-28 17:22
Alright, committed to 2.7, 3.2 an default.
Seems to work on all the buildbots, closing.
History
Date User Action Args
2011-08-28 17:22:43neologixsetstatus: open -> closed
resolution: fixed
messages: + msg143108

stage: patch review -> resolved
2011-08-28 16:09:16python-devsetmessages: + msg143101
2011-08-28 15:27:34neologixsetmessages: + msg143098
2011-08-28 14:32:29hayposetmessages: + msg143097
2011-08-28 14:21:58python-devsetmessages: + msg143096
2011-08-24 23:07:22hayposetmessages: + msg142947
2011-08-24 20:02:19pitrousetmessages: + msg142906
2011-08-24 19:40:36neologixsetmessages: + msg142904
2011-07-14 07:15:54neologixsetnosy: + brian.curtin
messages: + msg140323
2011-06-11 16:58:26python-devsetnosy: + python-dev
messages: + msg138170
2011-06-11 16:32:46pitrousetmessages: + msg138166
2011-06-11 16:30:22neologixsetmessages: + msg138165
2011-06-10 22:17:53neologixsetfiles: - oss_check_closed.diff
2011-06-10 22:17:44neologixsetfiles: - is_selectable.diff
2011-06-10 22:17:35neologixsetfiles: + oss_check_closed.diff
2011-06-10 22:17:01neologixsetfiles: + is_selectable.diff

messages: + msg138139
2011-06-10 21:57:27hayposetmessages: + msg138137
2011-06-10 21:54:41pitrousetmessages: + msg138136
2011-06-10 21:46:22neologixsetfiles: - oss_check_closed.diff
2011-06-10 21:46:11neologixsetfiles: - oss_select.diff
2011-06-10 21:45:27neologixsetfiles: + is_selectable.diff, oss_check_closed.diff

messages: + msg138135
2011-06-10 11:47:16amaury.forgeotdarcsetnosy: + amaury.forgeotdarc
messages: + msg138075
2011-06-09 22:53:10hayposetmessages: + msg138031
2011-06-09 18:13:23neologixsetfiles: + oss_check_closed.diff

messages: + msg138020
2011-06-08 21:53:06hayposetmessages: + msg137927
2011-06-08 20:43:57pitrousetmessages: + msg137924
2011-06-08 20:37:33neologixsetfiles: + test_oss.py
2011-06-08 20:37:14neologixcreate