adafruit / circuitpython

CircuitPython - a Python implementation for teaching coding with microcontrollers
https://circuitpython.org
Other
4.1k stars 1.22k forks source link

Struct module pack() bad argument check and out of bounds #7771

Closed Neradoc closed 1 year ago

Neradoc commented 1 year ago

CircuitPython version

Adafruit CircuitPython 8.1.0-beta.0-47-gc93560144 on 2023-03-22; Waveshare RP2040-Zero with rp2040

Code/REPL

>>> import struct
>>> struct.pack("2H", 1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: can't convert  to int
>>> struct.pack("2H", 1, 2)
b'\x01\x00\x02\x00'
>>> struct.pack("2H", 1, 2, 3)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
RuntimeError: too many arguments provided with the given format
>>> struct.pack("2H", 1)
b'\x01\x00\x02\x00'
>>> struct.pack("3H", 1, 4)
b'\x01\x00\x04\x00\x03\x00'

Behavior

The error when giving too few arguments is "can't convert to int". There is in fact no test in the code for the end of the list of arguments.

It looks like the inner loop for a format with a number like "4H" doesn't check for the number of arguments while reading and will (not so) happily read out of bounds. I guess that explains why having less arguments the second time does this, since it then reads the arguments from the previous call, which happen to be in the non allocated area of the stack.

No test for i < n_args: https://github.com/adafruit/circuitpython/blob/c93560144ba7c93d47ecd7743df9ada3c497a9de/shared-module/struct/__init__.c#L157-L165

A similar issue seems to exist in unpack_from but I didn't look into it.

Description

No response

Additional information

No response

jepler commented 1 year ago

In micropython they do this fwiw:

            // If we run out of args then we just finish; CPython would raise struct.error
            while (cnt-- && i < n_args) {
                mp_binary_set_val(fmt_type, *fmt, args[i++], p_base, &p);
            }

(so their behavior, of consistently getting 0-bytes in this case, is intentional)