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.

Sign up for GitHub

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

Jump to bottom

Check against PY_SSIZE_T_MAX instead of PY_SIZE_MAX in List_New #71849

Closed
zhangyangyu opened this issue Aug 1, 2016 · 15 comments
Closed

Check against PY_SSIZE_T_MAX instead of PY_SIZE_MAX in List_New #71849

zhangyangyu opened this issue Aug 1, 2016 · 15 comments
Assignees
@mdickinson
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement

Comments

@zhangyangyu
Copy link
Member

BPO 27662
Nosy @tim-one, @rhettinger, @mdickinson, @vadmium, @zhangyangyu
Files
  • List_New.patch
  • List_New_Calloc.patch
  • List_New_Calloc_v2.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 = 'https://github.com/mdickinson'
closed_at = <Date 2016-08-21.08:00:51.312>
created_at = <Date 2016-08-01.07:43:10.081>
labels = ['interpreter-core', 'type-feature']
title = 'Check against PY_SSIZE_T_MAX instead of PY_SIZE_MAX in List_New'
updated_at = <Date 2016-08-21.08:31:57.299>
user = 'https://github.com/zhangyangyu'

    bugs.python.org fields:

    activity = <Date 2016-08-21.08:31:57.299>
actor = 'python-dev'
assignee = 'mark.dickinson'
closed = True
closed_date = <Date 2016-08-21.08:00:51.312>
closer = 'mark.dickinson'
components = ['Interpreter Core']
creation = <Date 2016-08-01.07:43:10.081>
creator = 'xiang.zhang'
dependencies = []
files = ['43962', '44002', '44147']
hgrepos = []
issue_num = 27662
keywords = ['patch']
message_count = 15.0
messages = ['271774', '271798', '271801', '271804', '271944', '273086', '273088', '273091', '273094', '273095', '273096', '273097', '273277', '273278', '273281']
nosy_count = 6.0
nosy_names = ['tim.peters', 'rhettinger', 'mark.dickinson', 'python-dev', 'martin.panter', 'xiang.zhang']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue27662'
versions = ['Python 3.6']
    @zhangyangyu
    Copy link
    Member Author

    List_New checks against PY_SIZE_MAX. The upper bound of PyMem_Malloc is PY_SSIZE_T_MAX.

    Instead of simply changing the constant, another method is delegating overflow check to PyMem_Calloc, so we can avoid the check in unnecessary check in PyMem_Malloc. But I am not sure hiding the check in PyMem_Calloc is a good idea or not.

    @zhangyangyu zhangyangyu added interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement labels Aug 1, 2016
    @vadmium
    Copy link
    Member

    vadmium commented Aug 2, 2016

    It looks like PyMem_RESIZE() would be a truer equivalent than PyMem_Calloc(), since PyMem_MALLOC() does not initialize the memory. I would be happy with changing to that if you want.

    PyMem_Malloc() has been limited to PY_SSIZE_T_MAX since bpo-2620, although the documentation <https://docs.python.org/3.5/c-api/memory.html#c.PyMem_Malloc\> only mentions “size_t”. There is no match for “ssize_t” etc anywhere on that page.

    @zhangyangyu
    Copy link
    Member Author

    Thanks for your replies.

    I update the patch with PyMem_Calloc. I think it's better than PyMem_Resize since we need to initialize the memory here, there is a memset(op->ob_item, 0, nbytes) below.

    @rhettinger
    Copy link
    Contributor

    Mark, this is your code. Care to comment?

    @zhangyangyu
    Copy link
    Member Author

    Add another PY_SIZE_MAX replacement I suggest in listobject.c.

    @mdickinson
    Copy link
    Member

    The change to use PyMem_Calloc looks good to me.

    @mdickinson
    Copy link
    Member

    Also, if you're switching to PyMem_Calloc, I'd suggest dropping the assertion at the end, since that's now really an assertion for logic that appears in PyMem_Calloc rather than in this function.

    @zhangyangyu
    Copy link
    Member Author

    Thanks for your reply Mark. But I don't understand your message about the assertion. There is now no assertion in PyList_New. The changed assertion statement is in list_ass_subscript. If this confuses you I am quite sorry to bring in a somewhat unrelated change. :( I just thought it's not worth to open another issue to change that one. Really sorry.

    @mdickinson
    Copy link
    Member

    Right, sorry. I'm suggesting dropping that second assertion entirely; essentially, we're moving the responsibility for and knowledge about overflow checking into the PyMem_* functions/macros (which is where it belongs), and I don't see a need to check the equivalent condition in list_ass_subscript.

    If you do keep the second assertion, you should probably drop the (size_t) cast, so that we're not comparing signed with unsigned.

    @mdickinson
    Copy link
    Member

    There's also still a use of PY_SIZE_MAX in the list_resize function. Would it be worth fixing that one too?

    @zhangyangyu
    Copy link
    Member Author

    Just remove it. Regenerate the patch. As for list_resize, I have already fired another bpo-27660.

    @mdickinson
    Copy link
    Member

    LGTM. Thanks!

    I'll apply this later today, unless someone beats me to it.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 21, 2016

    New changeset cd3d079ad2b5 by Mark Dickinson in branch 'default':
    Issue bpo-27662: don't use PY_SIZE_MAX for overflow checking in List_New. Patch by Xiang Zhang.
    https://hg.python.org/cpython/rev/cd3d079ad2b5

    @mdickinson
    Copy link
    Member

    List_New_Calloc_v2.patch applied. Thanks!

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 21, 2016

    New changeset c7f9e66826a0 by Mark Dickinson in branch 'default':
    Issue bpo-27662: add missing Misc/NEWS entry.
    https://hg.python.org/cpython/rev/c7f9e66826a0

    @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
    Assignees

    @mdickinson mdickinson

    Labels
    interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    4 participants
    @mdickinson @vadmium @rhettinger @zhangyangyu