This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Undetected error in _struct.pack_into
Type: Stage:
Components: Extension Modules Versions: Python 3.0, Python 2.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: ajaksu2, amaury.forgeotdarc, georg.brandl, mark.dickinson, pitrou
Priority: normal Keywords: patch

Created on 2008-08-27 04:29 by ajaksu2, last changed 2022-04-11 14:56 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
s26.diff georg.brandl, 2008-08-27 08:47
s30.diff georg.brandl, 2008-08-27 08:47
pynumber_assizet_py3k.diff ajaksu2, 2009-02-11 15:04 PyLong_AsSsize_t -> PyNumber_AsSsize_t for 3.x
pynumber_assizet_trunk.diff ajaksu2, 2009-02-11 15:06 Test case for trunk
Messages (12)
msg72005 - (view) Author: Daniel Diniz (ajaksu2) * (Python triager) Date: 2008-08-27 04:29
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.
msg72014 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2008-08-27 08:44
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.
msg72015 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2008-08-27 08:47
The attached patches at least correct the XXX undetected error.
msg72016 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2008-08-27 09:26
Isn't PyNumber_AsSsize_t designed for this purpose?
msg81574 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-02-10 18:26
Yes, PyNumber_AsSsize_t() should be used instead.
msg81575 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-02-10 18:27
Oh, and a test should be added :)
msg81592 - (view) Author: Daniel Diniz (ajaksu2) * (Python triager) Date: 2009-02-10 19:59
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?
msg81600 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-02-10 21:08
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).
msg81648 - (view) Author: Daniel Diniz (ajaksu2) * (Python triager) Date: 2009-02-11 15:04
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?
msg81907 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2009-02-13 11:07
Applied in r69577, r69578.
msg81911 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-02-13 11:57
> 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?
msg82209 - (view) Author: Daniel Diniz (ajaksu2) * (Python triager) Date: 2009-02-16 02:43
>> 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: #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.
History
Date User Action Args
2022-04-11 14:56:38adminsetgithub: 47944
2009-02-16 02:43:22ajaksu2setmessages: + msg82209
2009-02-13 11:58:08mark.dickinsonsetmessages: + msg81911
2009-02-13 11:07:40georg.brandlsetstatus: open -> closed
resolution: fixed
messages: + msg81907
2009-02-11 15:06:32ajaksu2setfiles: + pynumber_assizet_trunk.diff
2009-02-11 15:04:47ajaksu2setfiles: - pynumber_assizet.diff
2009-02-11 15:04:20ajaksu2setfiles: + pynumber_assizet_py3k.diff
messages: + msg81648
2009-02-10 21:08:05pitrousetmessages: + msg81600
2009-02-10 19:59:50ajaksu2setfiles: + pynumber_assizet.diff
messages: + msg81592
2009-02-10 18:27:06pitrousetmessages: + msg81575
2009-02-10 18:26:29pitrousetnosy: + pitrou
messages: + msg81574
2009-02-10 18:13:59ajaksu2setnosy: + mark.dickinson
2008-08-27 09:26:28amaury.forgeotdarcsetnosy: + amaury.forgeotdarc
messages: + msg72016
2008-08-27 08:47:36georg.brandlsetfiles: + s30.diff
2008-08-27 08:47:30georg.brandlsetfiles: + s26.diff
keywords: + patch
messages: + msg72015
2008-08-27 08:44:42georg.brandlsetnosy: + georg.brandl
messages: + msg72014
2008-08-27 04:29:50ajaksu2create