This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Use of Py_ARRAY_LENGTH on pointer in posixmodule.c, win32_wchdir
Type: Stage: resolved
Components: Windows Versions: Python 3.4, Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Alexander Riccio, larry, paul.moore, python-dev, serhiy.storchaka, steve.dower, tim.golden, vstinner, zach.ware
Priority: normal Keywords:

Created on 2015-12-12 05:01 by Alexander Riccio, last changed 2022-04-11 14:58 by admin. This issue is now closed.

Messages (9)
msg256260 - (view) Author: Alexander Riccio (Alexander Riccio) * Date: 2015-12-12 05:01
I found this while writing up a separate bug (CPython doesn't use static analysis!).

In modules/posixmodule.c, win32_wchdir uses Py_ARRAY_LENGTH on a wchar_t*:

    wchar_t _new_path[MAX_PATH], *new_path = _new_path;
    int result;
    wchar_t env[4] = L"=x:";

    if(!SetCurrentDirectoryW(path))
        return FALSE;
    result = GetCurrentDirectoryW(Py_ARRAY_LENGTH(new_path), new_path);


...instead of using Py_ARRAY_LENGTH(_new_path), the programmer wrote Py_ARRAY_LENGTH(new_path), doesn't work on pointers:

/* Get the number of elements in a visible array

   This does not work on pointers, or arrays declared as [], or function
   parameters. With correct compiler support, such usage will cause a build
   error (see Py_BUILD_ASSERT_EXPR).

   Written by Rusty Russell, public domain, http://ccodearchive.net/
*/
#define Py_ARRAY_LENGTH(array) \
    (sizeof(array) / sizeof((array)[0]))


The same issue occurs two lines later:

    if (result > Py_ARRAY_LENGTH(new_path)) {



Compiling with /analyze found this quite easily:

c:\pythondev\repo\modules\posixmodule.c(1354): warning C6384: Dividing sizeof a pointer by another value.
msg256339 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-12-13 20:35
Oh, it's a regression introduced by:

changeset:   87516:46aecfc5e374
user:        Victor Stinner <victor.stinner@gmail.com>
date:        Sun Nov 24 19:23:25 2013 +0100
files:       Modules/posixmodule.c
description:
Issue #19636: Fix usage of MAX_PATH in posixmodule.c
msg256340 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-12-13 20:41
New changeset 39ce98d9b6b7 by Victor Stinner in branch '3.5':
Issue #25846: Fix usage of Py_ARRAY_LENGTH() in win32_wchdir()
https://hg.python.org/cpython/rev/39ce98d9b6b7

New changeset e373ec3c2969 by Victor Stinner in branch 'default':
(Merge 3.5) Issue #25846: Fix usage of Py_ARRAY_LENGTH() in win32_wchdir()
https://hg.python.org/cpython/rev/e373ec3c2969
msg256341 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-12-13 20:42
> I found this while writing up a separate bug (CPython doesn't use static analysis!).

Are you aware of the Coverity program? Last time I heard about Coverity, CPython had 0 bug found by Coverity ;-)
msg256343 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-12-13 21:01
> > I found this while writing up a separate bug (CPython doesn't use static analysis!).

The sad part is that Py_ARRAY_LENGTH() is written for static analysis, but it only works with some compilaters, and for example it doesn't work with Visual Studio, the only compiler on Windows officially supported.
msg256344 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-12-13 21:28
The other occurrence is not fixed.
msg256350 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-12-13 23:22
New changeset aba6caf7b9f0 by Victor Stinner in branch '3.5':
Issue #25846: Fix usage of Py_ARRAY_LENGTH() in win32_wchdir() (new try)
https://hg.python.org/cpython/rev/aba6caf7b9f0
msg256351 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-12-13 23:23
> The other occurrence is not fixed.

Oops, right. Good catch. This is what happens when I skip the code review step :-/ Is it better with the second change?
msg256372 - (view) Author: Alexander Riccio (Alexander Riccio) * Date: 2015-12-14 06:54
> Are you aware of the Coverity program? Last time I heard about Coverity, CPython had 0 bug found by Coverity ;-)

Yup, see Issue25847.

> The sad part is that Py_ARRAY_LENGTH() is written for static analysis

Sadly, yeah. MSVC, when compiling as C++, should actually catch this, but CPython precludes that.
History
Date User Action Args
2022-04-11 14:58:24adminsetgithub: 70033
2015-12-18 17:45:54serhiy.storchakasetstatus: open -> closed
resolution: fixed
stage: resolved
2015-12-14 06:54:49Alexander Ricciosettype: behavior ->
resolution: fixed -> (no value)
messages: + msg256372
2015-12-13 23:23:05vstinnersetmessages: + msg256351
versions: + Python 3.4, Python 3.5
2015-12-13 23:22:31python-devsetmessages: + msg256350
2015-12-13 21:28:29serhiy.storchakasetstatus: closed -> open
nosy: + serhiy.storchaka
messages: + msg256344

2015-12-13 21:01:46vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg256343
2015-12-13 20:42:16vstinnersetmessages: + msg256341
2015-12-13 20:41:21python-devsetnosy: + python-dev
messages: + msg256340
2015-12-13 20:35:31vstinnersetmessages: + msg256339
2015-12-12 10:10:44serhiy.storchakasetnosy: + vstinner
type: behavior
2015-12-12 05:01:29Alexander Ricciocreate