string parameter to ioctl not null terminated, includes fix

[qbarnes]

BPO	1433877
Nosy	@mwhudson, @gvanrossum, @Yhg1s
Files	fcntlmodule.c.patch: patch file for fcntlmodule.c bug fcntlmodule.c-take2.patch: Reworked patch for fcntlmodule.c

^{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/Yhg1s'
closed_at = <Date 2006-04-25.13:59:25.000>
created_at = <Date 2006-02-17.22:29:55.000>
labels = ['extension-modules']
title = 'string parameter to ioctl not null terminated, includes fix'
updated_at = <Date 2006-04-25.13:59:25.000>
user = 'https://bugs.python.org/qbarnes'

bugs.python.org fields:

activity = <Date 2006-04-25.13:59:25.000>
actor = 'twouters'
assignee = 'twouters'
closed = True
closed_date = None
closer = None
components = ['Extension Modules']
creation = <Date 2006-02-17.22:29:55.000>
creator = 'qbarnes'
dependencies = []
files = ['1902', '1903']
hgrepos = []
issue_num = 1433877
keywords = []
message_count = 11.0
messages = ['27552', '27553', '27554', '27555', '27556', '27557', '27558', '27559', '27560', '27561', '27562']
nosy_count = 5.0
nosy_names = ['mwh', 'gvanrossum', 'twouters', 'nnorwitz', 'qbarnes']
pr_nums = []
priority = 'high'
resolution = 'fixed'
stage = None
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue1433877'
versions = ['Python 2.4']

[qbarnes]

I ran across this problem in Python 2.3.3 and see it is
still there in 2.4.2.

When running the test_pty.py test case, I would get
intermittant failures depending on how the test was
invoked or the compiler flags used on Solaris.
Trussing the interpreter while running the test revealed:
ioctl(4, I_PUSH, "ptem\xff^T^F^H") Err#22 EINVAL

There was garbage on the end of the string when it
would fail.

I tracked the problem back to fcntl_ioctl() in
fcntlmodule.c. The string was not being NULL
terminated, but relied on having zeroed gargage on the
stack to work.

I checked the source for 2.4.2 and noticed the same
problem. Patch to fix the problem is attached.

[nnorwitz]

Logged In: YES
user_id=33168

Thomas, could this have caused the flakiness that you just
fixed with test_pty? This patch seems correct to me and I
think it should be applied. (If you want I can do that, but
I wanted you to see this first.)

[gvanrossum]

Logged In: YES
user_id=6380

One problem with ioctl() is that the required length of the
buffer is unknown unless you parse the operation code
argument in a very platform- (and probably even kernel- and
driver-) dependent way. Whether null-terminating it makes
sense or not depends on the particular operation code.

I don't think the patch makes ioctl "safe", and I'm not sure
it even ought to be applied. A quick code review reveals a
few more issues:

• the docstring doesn't explain the optional "mutate_flag"
  argument (which BTW is a mess -- read the online docs -- why
  are we changing the default to true?)

• the online docs ought to be updated for 2.4 and again for
  2.5 -- they still speak of 2.4 in the future tense! Also,
  it's a bit strong to say this function is "identical" to
  fcntl() -- "similar" sounds more like it.

• In the first branch of the function, this line:

    if (mutate_arg && (len < sizeof buf)) {

ought to test (len <= sizeof buf) to match the test earlier;
but probably better is to use

    if (mutate_arg && arg == buf) {

• If it's important to null-terminate the data in the buffer
  when a string is passed in, shouldn't we null-terminate it
  also when a writable buffer-interface object is passed in?
  If not in the latter case, why if a string is passed? One
  could argue that a string with an explicit \0 (visible to
  Python code) at the end ought to be passed in if the
  semantics of the op code requires a null-terminated argument
  (which most ioctl op codes don't -- the typical argument is
  a C struct).

• The proposed patch reduces the maximum buffer size to
  1023, which violates the docs. (Yes the docs explicitly
  mention 1024.)

• The best we can do seems to be: increase the buffer size
  to 1025; null-pad it in all cases; change the code for
  mutable buffers to test for sizeof buf - 1. This still
  leaves the case where the buffer isn't used because a
  mutable buffer is passed that's longer than 1024. Which
  suggests that we can't completely fix this -- it will always
  be possible to crash Python using this function by passing
  an op code that encodes a buffer length > 1024 while passing
  a shorter argument, so the internal buffer is used. I guess
  we could do the null-padding as long as we document it and
  document that when a mutable buffer is passed we don't
  guarantee it.

[qbarnes]

Logged In: YES
user_id=288734

I've practically never used python. I just found
this bug in the interpreter while porting the
tool to the current Solaris compiler release, so please
keep that in mind about my lack of python knowledge.
I also have no idea who's who as well in the python
world so forgive me if I've stepped on anyone's
toes.

I don't follow the remark about "making ioctl
'safe'". I never said anything about making it
"safe". It's about a bug.

I also never said anything about the interpreter
crashing. The interpreter never crashed. The
pty test just simply failed since the module
name passed to the OS's ioctl was corrupted.

Are you sure you responded to the right bug
report with these remarks?

Anyways...

This problem should be reduced to a classic
data marshaling problem between python data
space and C data space which should have already
been addressed in many contexts many times before
in the interpreter. How are python strings
converted for use by C in similar situations?

This problem I encountered is either a bug in
the code that passed the string to the ioctl
service by not putting an explicit '\0' on the
end of the "ptem" string, or it is a bug in the
fcntlmodule.c which should have done it for it.
Which is it?

If a python code isn't expected to put a literal
null at the end of their string when passing to
a C service, then ioctl needs to be fixed similar
to the way in my patch.

As for null-terminating other raw data, I don't
see the point other than a defensive programming
practice.

The problem I ran across is limited to just
a data marshaling problem. The patch only
touched the case when the type of data passed
to ioctl is known to be a string.

As for the buffer vs. documentation, the
buffer could be changed to 1025 for the string
case, or the documentation could be updated
to clarify that, unlike raw data, strings are
limited to 1023 to leave room for the
requisite null that conversion to a C string
requires. I don't think anyone would care
either way as long as it is qualified.

Since python strings serve dual roles of being
both raw data and being just textual containers,
one can't assume that a string passed to ioctl
is always meant to be just a textual string. Going
the 1025 route is probably a 'better' approach due to
python string's duality.

[gvanrossum]

Logged In: YES
user_id=6380

Sorry for the confusion. I was in part responding to an
off-line discussion I had with Neal Norwitz, to which you
weren't a party. You can ignore my comments about safety and
crashing.

The difference in marshalling Python data to C data for
ioctl(), compared to the many other places where (indeed)
this problem has been solved before, is that almost
everywhere else, we *know* what the data is supposed to
mean. C has many more data types than Python, and the
choice of how to convert a string, or an int, to C data
depends on what is expected by the receiver of the C data.
Unfortunately for ioctl we don't know the required C data
type because it's defined by the kernel module that handles
the particular ioctl opcode. Some of these (like the one
you apparently ran into when porting the pty code to
Solaris) require a null-terminated string; others (like the
classic tty ioctls) require a C structure, which in Python
can be created by using the struct module. Both return
Python strings.

I tend to agree with your conclusion that we should extend
the buffer to 1025 bytes and not bother null-terminating
non-string data. Would you like to attempt another patch?

[mwhudson]

Logged In: YES
user_id=6656

Seeing as some of this is my code...

Guido, the ioctl docstring *does* mention mutate_arg. I agree that the
documentation should have been updated for 2.4 and 2.5... and the situation
is a mess, yes, but one that I couldn't see a better way around when the
feature was added (it was much discussed in the bug report at the time).

It certainly never occurred to me that you might need/want to pass a NULL
terminated string to ioctl.

I don't think this is a priority 9 report.

[qbarnes]

Logged In: YES
user_id=288734

I didn't write new code that causes this problem by calling
ioctl with a string that needed to be null terminated. It
was already in Python code itself. See Lib/pty.py for the
code.

I reworked the patch as discussed below and retested it.
All built-in tests passed.

I've attached it. I hae no idea of Python coding
conventions or style. Hopefully I didn't violate them
too badly.

(This was weird. SF rejected the text above when I
submitted it, but it took the patch file. The patch
file shows up as "nobody". Resubmitting this text.)

[gvanrossum]

Logged In: YES
user_id=6380

mwh: Sorry about the docstring gripe -- I read the fcntl()
docstring which is right above the ioctl() implementation
and never realized that this particular C module places the
doc strings *after* the functions. (I think that's bad style
but there it is.)

I think the priority was set to 9 by Neal to get Thomas'
attention.

gbarnes: We'll decide this one at PyCon.

[Yhg1s]

Logged In: YES
user_id=34209

Huh, I never even saw this patch, in spite of the high
priority and it being assigned to me. <insert comment about
SF bugtracker>

We didn't discuss it at PyCon (at least, I wasn't party to
the discussion) but I guess this patch doesn't really hurt,
and does fix actual problems. I'm wary of fudging
fcntl/ioctl too much; I'd _much_ rather try and come up with
a decent interface for Py3k, with mutable bytestrings and
all that, maybe some nice automatic argument-type-conversion
or higher-level interface for common features (like advisory
locks) -- and only keep 2.x's fcntl working as well as it does.

[Yhg1s]

Logged In: YES
user_id=34209

Hrm. After giving this some thought, perhaps we should do

 memcpy(buf, str, len+1)

instead of

 memcpy(buf, str, len)
 buf[len] = '\0'

(And do this in the writable-buffer case as well, when we
use memcpy.) Other than that, I think this is the right fix.
Assigning to mwh for second (third? fourth?) opinion.

[Yhg1s]

Logged In: YES
user_id=34209

Checked this in before 2.5 alpha2, after more contemplation.
Undid the circumventing of the test_pty problems, too.
Decided to explicitly set the terminating NUL after all,
instead of relying on the given buffer to have one (even
though it really should), and terminated the array in the
writable-buffer case, too (it may not matter, but it
certainly doesn't hurt, since we didn't end up copying any
data there in any case.) Thanks for figuring it out, Quentin.