Closed
Description
BPO | 24823 |
---|---|
Nosy | @terryjreedy, @amauryfa, @abalkin, @ezio-melotti, @meadori, @eryksun, @pohlt, @willingc |
Files |
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']
Activity
pohlt commentedon Aug 7, 2015
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
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 commentedon Aug 7, 2015
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 commentedon Aug 7, 2015
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.
pohlt commentedon Aug 7, 2015
If one needs to set a general buffer (i.e. not a null-terminated string buffer) one could always use:
krista commentedon Jan 9, 2016
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 commentedon Jan 9, 2016
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.
terryjreedy commentedon Jan 18, 2016
(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