Skip to content

gh-127182: Fix io.StringIO.__setstate__ crash when None is the first value #127219

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.

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

Merged
merged 3 commits into from
Nov 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions Lib/test/test_io.py
Original file line number Diff line number Diff line change
Expand Up @@ -1148,6 +1148,21 @@ def test_disallow_instantiation(self):
_io = self._io
support.check_disallow_instantiation(self, _io._BytesIOBuffer)

def test_stringio_setstate(self):
# gh-127182: Calling __setstate__() with invalid arguments must not crash
obj = self._io.StringIO()
with self.assertRaisesRegex(
TypeError,
'initial_value must be str or None, not int',
):
obj.__setstate__((1, '', 0, {}))

obj.__setstate__((None, '', 0, {})) # should not crash
self.assertEqual(obj.getvalue(), '')

obj.__setstate__(('', '', 0, {}))
self.assertEqual(obj.getvalue(), '')

class PyIOTest(IOTest):
pass

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fix :meth:`!io.StringIO.__setstate__` crash, when :const:`None` was passed as
the first value.
30 changes: 16 additions & 14 deletions Modules/_io/stringio.c
Original file line number Diff line number Diff line change
Expand Up @@ -908,23 +908,25 @@ _io_StringIO___setstate___impl(stringio *self, PyObject *state)
once by __init__. So we do not take any chance and replace object's
buffer completely. */
{
PyObject *item;
Py_UCS4 *buf;
Py_ssize_t bufsize;

item = PyTuple_GET_ITEM(state, 0);
buf = PyUnicode_AsUCS4Copy(item);
if (buf == NULL)
return NULL;
bufsize = PyUnicode_GET_LENGTH(item);
PyObject *item = PyTuple_GET_ITEM(state, 0);
if (PyUnicode_Check(item)) {
Py_UCS4 *buf = PyUnicode_AsUCS4Copy(item);
if (buf == NULL)
return NULL;
Py_ssize_t bufsize = PyUnicode_GET_LENGTH(item);

if (resize_buffer(self, bufsize) < 0) {
if (resize_buffer(self, bufsize) < 0) {
PyMem_Free(buf);
return NULL;
}
memcpy(self->buf, buf, bufsize * sizeof(Py_UCS4));
PyMem_Free(buf);
return NULL;
self->string_size = bufsize;
}
else {
assert(item == Py_None);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be worth it to check item value at runtime, and fail with an assertion if it's not None.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that it is, because __init__ call above checks that. I don't think that it would be possible to trigger this error here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right, it makes sense. I didn't see the _io_StringIO___init__() call.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a side note, should we encourage using Py_Is instead of ==?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, Py_IsNone() can be used ;-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to keep it as == Py_None, because other related parts do the same thing: if (value && value != Py_None && !PyUnicode_Check(value)) in __init__, for example.

self->string_size = 0;
}
memcpy(self->buf, buf, bufsize * sizeof(Py_UCS4));
PyMem_Free(buf);
self->string_size = bufsize;
}

/* Set carefully the position value. Alternatively, we could use the seek
Expand Down
Loading