[ python-Bugs-1433877 ] string parameter to ioctl not null terminated, includes fix

[SourceForge.net]

Bugs item #1433877, was opened at 2006-02-17 17:29
Message generated for change (Comment added) made by gvanrossum
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=105470&aid=1433877&group_id=5470

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: Extension Modules
Group: Python 2.4
Status: Open
Resolution: None
Priority: 9
Submitted By: Quentin Barnes (qbarnes)
Assigned to: Thomas Wouters (twouters)
Summary: string parameter to ioctl not null terminated, includes fix

Initial Comment:
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.


----------------------------------------------------------------------

>Comment By: Guido van Rossum (gvanrossum)
Date: 2006-02-19 15:03

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

----------------------------------------------------------------------

Comment By: Neal Norwitz (nnorwitz)
Date: 2006-02-18 02:14

Message:
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.)

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=105470&aid=1433877&group_id=5470