Skip to content

ctypes.create_string_buffer does not add NUL if len(init) == size #69011

Closed
@pohlt

Description

@pohlt
mannequin
BPO 24823
Nosy @terryjreedy, @amauryfa, @abalkin, @ezio-melotti, @meadori, @eryksun, @pohlt, @willingc
Files
  • create_string_buffer.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 = None
    closed_at = None
    created_at = <Date 2015-08-07.10:18:08.042>
    labels = ['easy', 'type-bug', '3.8', '3.9', '3.10', 'ctypes', 'library']
    title = 'ctypes.create_string_buffer does not add NUL if len(init) == size'
    updated_at = <Date 2021-03-19.01:57:45.139>
    user = 'https://github.com/pohlt'

    bugs.python.org fields:

    activity = <Date 2021-03-19.01:57:45.139>
    actor = 'eryksun'
    assignee = 'docs@python'
    closed = False
    closed_date = None
    closer = None
    components = ['Library (Lib)', 'ctypes']
    creation = <Date 2015-08-07.10:18:08.042>
    creator = 'tom.pohl'
    dependencies = []
    files = ['41558']
    hgrepos = []
    issue_num = 24823
    keywords = ['patch', 'easy']
    message_count = 8.0
    messages = ['248183', '248211', '248219', '248222', '257862', '257876', '258561', '389050']
    nosy_count = 10.0
    nosy_names = ['terry.reedy', 'amaury.forgeotdarc', 'belopolsky', 'ezio.melotti', 'meador.inge', 'docs@python', 'eryksun', 'tom.pohl', 'willingc', 'krista']
    pr_nums = []
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue24823'
    versions = ['Python 3.8', 'Python 3.9', 'Python 3.10']

    Linked PRs

    Activity

    pohlt

    pohlt commented on Aug 7, 2015

    @pohlt
    MannequinAuthor

    From the ctypes.create_string_buffer docs:
    """If a bytes object is specified as first argument, the buffer is made one item larger than its length so that the last element in the array is a NUL termination character. An integer can be passed as second argument which allows to specify the size of the array if the length of the bytes should not be used."""

    Based on this documentation I would expect a NUL-terminated byte array in any case. However, when I do this

    >>> for size in range(5, 2, -1): print(size, ctypes.create_string_buffer(b'123', size).raw)
    5 b'123\x00\x00'
    4 b'123\x00'
    3 b'123'

    I get b'123' for size=3 without a NUL. My expectation would be the same exception as I get for create_string_buffer(b'123', 2).

    eryksun

    eryksun commented on Aug 7, 2015

    @eryksun
    Contributor

    Not every buffer is null-terminated. That's just the assumption used if the size isn't specified. The documentation can possibly be reworded to make this clearer, but the function itself shouldn't be changed.

    pohlt

    pohlt commented on Aug 7, 2015

    @pohlt
    MannequinAuthor

    I agree: not every buffer is null-terminated.

    But the function name suggests that it creates a _string_ buffer which will most likely be used as an input to a C function. There, it can easily trigger a buffer overflow without a null termination which can be considered a severe security risk.

    removed
    docsDocumentation in the Doc dir
    on Aug 7, 2015
    pohlt

    pohlt commented on Aug 7, 2015

    @pohlt
    MannequinAuthor

    If one needs to set a general buffer (i.e. not a null-terminated string buffer) one could always use:

    >> string = (ctypes.c_char*4)()
    >> string.raw = b'abcd'

    krista

    krista commented on Jan 9, 2016

    @krista
    Mannequin

    Patch containing checking for buffer size, so that NULL value is the last byte as C standard specifies. Raises ValueError exception if initial value does not fit into to the buffer with NULL char.

    This should decrease the possibility of creating security issues.

    eryksun

    eryksun commented on Jan 9, 2016

    @eryksun
    Contributor

    I didn't want to change the function in lieu of breaking someone's code. If this change is accepted, then it at least needs a documentation note to indicate the new behavior.

    added
    docsDocumentation in the Doc dir
    stdlibPython modules in the Lib dir
    on Jan 9, 2016
    terryjreedy

    terryjreedy commented on Jan 18, 2016

    @terryjreedy
    Member

    (Tracker notes:

    I added as nosy the people listed as active 'experts' for ctypes on https://docs.python.org/devguide/experts.html#experts. This was easily done by going to the end of the nosy list, typing a comma ',', typing 'ctypes', and then clicking the box that appeared. This can be done for any module and the other topics listed on the page.

    The Documentation component is for issues that only change the docs, and not the code. That is why Documentation issues are auto-assigned to docs@python. Adding 'Documentation' amounts to rejecting this patch or anything else that changes the code.

    asyncio, ctypes, IDLE (idlelib), IO, and (T)tkinter are all parts of the stdlib and AFAIK, issues marked for them do not have to also be marked 'Library'.)
    ---

    I looked at ctypes.py with hg annotate. Create_string_buffer is part of Thomas Heller's original 2006-03-08 patch that moved ctypes from an external source into the stdlib. The only changes are in the isinstance class checks and the raise statement; the conditional bodies, including the one in question, are unchanged.

    Tom, we disagree on our reading of the current docs. The default number of NULL bytes added is 1. Is the second argument required to be large enough to keep the number positive? You think yes, I think no, though I agree with Eryk that the second quoted sentence could and should be clearer. I will not assume that T. Heller meant 'yes' when he wrote 'no' in the code. What do the listed experts think?

    If the doc matches the code, there is no implementation bug and this is not a behavior issue. It is still possible to request a design change as an enhancement. I think this would require agreement of at least two core developers. A deprecation notice would normally be needed. A third possibility is to decide that this is a security issue severe enough to possibly break code in 3.6 and possibly sooner. I think this would require pydev discussion.

    One problem with changing ctypes is that it is not used in the stdlib, so we have no local examples to draw on. In this case, the question would be how often is 'size' used to suppress the default NULL byte and how legitimate are such uses.

    24 remaining items

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Metadata

    Metadata

    Assignees

    No one assigned

      Labels

      3.10only security fixes3.8 (EOL)end of life3.9only security fixeseasystdlibPython modules in the Lib dirtopic-ctypestype-bugAn unexpected behavior, bug, or error

      Projects

      No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions

        ctypes.create_string_buffer does not add NUL if len(init) == size · Issue #69011 · python/cpython

        Follow Lee on X/Twitter - Father, Husband, Serial builder creating AI, crypto, games & web tools. We are friends :) AI Will Come To Life!

        Check out: eBank.nz (Art Generator) | Netwrck.com (AI Tools) | Text-Generator.io (AI API) | BitBank.nz (Crypto AI) | ReadingTime (Kids Reading) | RewordGame | BigMultiplayerChess | WebFiddle | How.nz | Helix AI Assistant