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

Use of Py_ARRAY_LENGTH on pointer in posixmodule.c, win32_wchdir #70033

Closed
ariccio mannequin opened this issue Dec 12, 2015 · 9 comments
Closed

Use of Py_ARRAY_LENGTH on pointer in posixmodule.c, win32_wchdir #70033

ariccio mannequin opened this issue Dec 12, 2015 · 9 comments

Comments

@ariccio
Copy link
Mannequin

ariccio mannequin commented Dec 12, 2015

BPO 25846
Nosy @pfmoore, @vstinner, @larryhastings, @tjguk, @zware, @serhiy-storchaka, @zooba, @ariccio

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 = <Date 2015-12-18.17:45:54.962>
created_at = <Date 2015-12-12.05:01:29.839>
labels = ['OS-windows']
title = 'Use of Py_ARRAY_LENGTH on pointer in posixmodule.c, win32_wchdir'
updated_at = <Date 2015-12-18.17:45:54.961>
user = 'https://github.com/ariccio'

bugs.python.org fields:

activity = <Date 2015-12-18.17:45:54.961>
actor = 'serhiy.storchaka'
assignee = 'none'
closed = True
closed_date = <Date 2015-12-18.17:45:54.962>
closer = 'serhiy.storchaka'
components = ['Windows']
creation = <Date 2015-12-12.05:01:29.839>
creator = 'Alexander Riccio'
dependencies = []
files = []
hgrepos = []
issue_num = 25846
keywords = []
message_count = 9.0
messages = ['256260', '256339', '256340', '256341', '256343', '256344', '256350', '256351', '256372']
nosy_count = 9.0
nosy_names = ['paul.moore', 'vstinner', 'larry', 'tim.golden', 'python-dev', 'zach.ware', 'serhiy.storchaka', 'steve.dower', 'Alexander Riccio']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue25846'
versions = ['Python 3.4', 'Python 3.5']

@ariccio
Copy link
Mannequin Author

ariccio mannequin commented Dec 12, 2015

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.

@ariccio ariccio mannequin added the OS-windows label Dec 12, 2015
@serhiy-storchaka serhiy-storchaka added the type-bug An unexpected behavior, bug, or error label Dec 12, 2015
@vstinner
Copy link
Member

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 bpo-19636: Fix usage of MAX_PATH in posixmodule.c

@python-dev
Copy link
Mannequin

python-dev mannequin commented Dec 13, 2015

New changeset 39ce98d9b6b7 by Victor Stinner in branch '3.5':
Issue bpo-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 bpo-25846: Fix usage of Py_ARRAY_LENGTH() in win32_wchdir()
https://hg.python.org/cpython/rev/e373ec3c2969

@vstinner
Copy link
Member

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 ;-)

@vstinner
Copy link
Member

> 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.

@serhiy-storchaka
Copy link
Member

The other occurrence is not fixed.

@python-dev
Copy link
Mannequin

python-dev mannequin commented Dec 13, 2015

New changeset aba6caf7b9f0 by Victor Stinner in branch '3.5':
Issue bpo-25846: Fix usage of Py_ARRAY_LENGTH() in win32_wchdir() (new try)
https://hg.python.org/cpython/rev/aba6caf7b9f0

@vstinner
Copy link
Member

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?

@ariccio
Copy link
Mannequin Author

ariccio mannequin commented Dec 14, 2015

Are you aware of the Coverity program? Last time I heard about Coverity, CPython had 0 bug found by Coverity ;-)

Yup, see bpo-25847.

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.

@ariccio ariccio mannequin removed the type-bug An unexpected behavior, bug, or error label Dec 14, 2015
@ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants