Undetected error in _struct.pack_into

[devdanzin]

BPO	3694
Nosy	@birkenfeld, @amauryfa, @mdickinson, @pitrou, @devdanzin
Files	s26.diff s30.diff pynumber_assizet_py3k.diff: PyLong_AsSsize_t -> PyNumber_AsSsize_t for 3.x pynumber_assizet_trunk.diff: Test case for trunk

^{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 = <Date 2009-02-13.11:07:40.661>
created_at = <Date 2008-08-27.04:29:50.746>
labels = ['extension-modules']
title = 'Undetected error in _struct.pack_into'
updated_at = <Date 2009-02-16.02:43:22.530>
user = 'https://github.com/devdanzin'

bugs.python.org fields:

activity = <Date 2009-02-16.02:43:22.530>
actor = 'ajaksu2'
assignee = 'none'
closed = True
closed_date = <Date 2009-02-13.11:07:40.661>
closer = 'georg.brandl'
components = ['Extension Modules']
creation = <Date 2008-08-27.04:29:50.746>
creator = 'ajaksu2'
dependencies = []
files = ['11267', '11268', '13027', '13028']
hgrepos = []
issue_num = 3694
keywords = ['patch']
message_count = 12.0
messages = ['72005', '72014', '72015', '72016', '81574', '81575', '81592', '81600', '81648', '81907', '81911', '82209']
nosy_count = 5.0
nosy_names = ['georg.brandl', 'amaury.forgeotdarc', 'mark.dickinson', 'pitrou', 'ajaksu2']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = None
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue3694'
versions = ['Python 2.6', 'Python 3.0']

[devdanzin]

The following code leads to XXX Undetected errors in debug builds of
trunk and 3.0:

import _struct
_struct.pack_into(b"8", bytearray(1), None)

Besides that, there's something fishy happening in non-debug builds:

2.6:
>>> _struct.pack_into(b"8", bytearray(1), None);
>>> _struct.pack_into(b"8", bytearray(1), None);
>>> import sys
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: an integer is required

3.0:
>>> _struct.pack_into(b"8", bytearray(1), None)
>>> _struct.pack_into(b"8", bytearray(1), None)
SystemError: Objects/longobject.c:433: bad argument to internal function

Found with Fusil.

[birkenfeld]

The problem is that, unlike PyInt_AsSsize_t, PyLong_AsSsize_t expects
its argument to already be a long object and else raises the SystemError.

It should probably behave like PyInt_AsSsize_t and raise the TypeError
since in 3.0 it's used in many places where PyInt_AsSsize_t was used.

[birkenfeld]

The attached patches at least correct the XXX undetected error.

[amauryfa]

Isn't PyNumber_AsSsize_t designed for this purpose?

[pitrou]

Yes, PyNumber_AsSsize_t() should be used instead.

[pitrou]

Oh, and a test should be added :)

[devdanzin]

Here's a patch with test for 3.x. Erm, I have no idea if that's all that
is necessary :)

Does this have the potential to break existing code?

[pitrou]

Well, a patch for 2.6 should be provided as well. Besides,
PyExc_OverflowError is probably a better choice than PyExc_IndexError
(but I'm not sure on this one).

[devdanzin]

Looks like both Undetected errors were corrected by Victor's patches,
which Benjamin committed around rev66693, so trunk only needs a test.
Here are the patches.

I think IndexError fits better (and matches trunk), as the issue is that
None is passed as "offset", which should be an Integral. If you
disagree, I'll modify the patch. Is PyErr_Format'ing the exception desired?

BTW, there's a warning in _struct.c:180 -> warning: ‘get_ulong’ defined
but not used, should I open a new issue?

[birkenfeld]

Applied in r69577, r69578.

[mdickinson]

BTW, there's a warning in _struct.c:180 -> warning: ‘get_ulong’
defined but not used, should I open a new issue?

Sure. Please could you add me to the nosy list if you do.

In my opinion, the struct module *really* needs an overhaul, especially
for py3k; there's a lot of inconsistency in the behaviour with respect
to different integer types, and there's a lot of code that seems to be
there purely for backwards compatibility that could be removed for 3.1
(and probably for 2.7 as well): for example, allowing floats when
packing integer types, and allowing overflow to wraparound rather than
raising an exception.

Would it be worth opening a general 'overhaul the struct module' issue
and marking all the current struct bugs as dependencies for this issue?

[devdanzin]

> BTW, there's a warning in _struct.c:180 -> warning: 'get_ulong'
> defined but not used, should I open a new issue?

Sure. Please could you add me to the nosy list if you do.

OK, should do that soon.

In my opinion, the struct module *really* needs an overhaul, especially
for py3k; there's a lot of inconsistency in the behaviour with respect
to different integer types, and there's a lot of code that seems to be
there purely for backwards compatibility that could be removed for 3.1
(and probably for 2.7 as well): for example, allowing floats when
packing integer types, and allowing overflow to wraparound rather than
raising an exception.

This one looks bad: bpo-2590.

Would it be worth opening a general 'overhaul the struct module' issue
and marking all the current struct bugs as dependencies for this issue?

I can't judge on the merit of struct's shortcomings, but I'll propose
the 'umbrella issue' idea for other targets (socket, HTMLParser,
etc.) on python-dev and can suggest one for struct too.