From 54d97251fe2dd4939652a186bf703885e654b4d1 Mon Sep 17 00:00:00 2001 From: Jeff Epler Date: Sat, 12 Sep 2020 13:57:31 -0500 Subject: [PATCH 1/8] modstruct: Improve compliance with python3 While checking whether we can enable -Wimplicit-fallthrough, I encountered a diagnostic in mp_binary_set_val_array_from_int which led to discovering the following bug: ``` >>> struct.pack("xb", 3) b'\x03\x03' ``` That is, the next value (3) was used as the value of a padding byte, while standard Python always fills "x" bytes with zeros. I initially thought this had to do with the unintentional fallthrough, but it doesn't. Instead, this code would relate to an array.array with a typecode of padding ('x'), which is ALSO not desktop Python compliant: ``` >>> array.array('x', (1, 2, 3)) array('x', [1, 0, 0]) ``` Possibly this is dead code that used to be shared between struct-setting and array-setting, but it no longer is. I also discovered that the argument list length for struct.pack and struct.pack_into were not checked, and that the length of binary data passed to array.array was not checked to be a multiple of the element size. I have corrected all of these to conform more closely to standard Python and revised some tests where necessary. Some tests for micropython-specific behavior that does not conform to standard Python and is not present in CircuitPython was deleted outright. --- py/binary.c | 9 +++--- py/modstruct.c | 17 ++++++----- py/objarray.c | 7 ++++- tests/basics/struct1.py | 25 ++++++++++++++++ tests/basics/struct_micropython.py | 41 -------------------------- tests/basics/struct_micropython.py.exp | 2 -- tests/misc/non_compliant.py | 6 ---- tests/misc/non_compliant.py.exp | 2 -- 8 files changed, 46 insertions(+), 63 deletions(-) delete mode 100644 tests/basics/struct_micropython.py delete mode 100644 tests/basics/struct_micropython.py.exp diff --git a/py/binary.c b/py/binary.c index cd0f1aa4df..b85edba625 100644 --- a/py/binary.c +++ b/py/binary.c @@ -126,7 +126,6 @@ mp_obj_t mp_binary_get_val_array(char typecode, void *p, mp_uint_t index) { break; case BYTEARRAY_TYPECODE: case 'B': - case 'x': // value will be discarded val = ((unsigned char*)p)[index]; break; case 'h': @@ -330,7 +329,11 @@ void mp_binary_set_val(char struct_type, char val_type, mp_obj_t val_in, byte ** } } - mp_binary_set_int(MIN((size_t)size, sizeof(val)), struct_type == '>', p, val); + if (val_type == 'x') { + memset(p, 0, 1); + } else { + mp_binary_set_int(MIN((size_t)size, sizeof(val)), struct_type == '>', p, val); + } } void mp_binary_set_val_array(char typecode, void *p, mp_uint_t index, mp_obj_t val_in) { @@ -379,8 +382,6 @@ void mp_binary_set_val_array_from_int(char typecode, void *p, mp_uint_t index, m case 'B': ((unsigned char*)p)[index] = val; break; - case 'x': - ((unsigned char*)p)[index] = 0; case 'h': ((short*)p)[index] = val; break; diff --git a/py/modstruct.c b/py/modstruct.c index fe766a4deb..7675de275d 100644 --- a/py/modstruct.c +++ b/py/modstruct.c @@ -183,16 +183,21 @@ MP_DEFINE_CONST_FUN_OBJ_VAR_BETWEEN(struct_unpack_from_obj, 2, 3, struct_unpack_ // This function assumes there is enough room in p to store all the values STATIC void struct_pack_into_internal(mp_obj_t fmt_in, byte *p, size_t n_args, const mp_obj_t *args) { + size_t size; + size_t count = calc_size_items(mp_obj_str_get_str(fmt_in), &size); + if (count != n_args) { +#if MICROPY_ERROR_REPORTING == MICROPY_ERROR_REPORTING_TERSE + mp_raise_ValueError(NULL); +#else + mp_raise_ValueError_varg(translate("pack expected %d items for packing (got %d)"), count, n_args); +#endif + } const char *fmt = mp_obj_str_get_str(fmt_in); char fmt_type = get_fmt_type(&fmt); size_t i; for (i = 0; i < n_args;) { mp_uint_t cnt = 1; - if (*fmt == '\0') { - // more arguments given than used by format string; CPython raises struct.error here - break; - } if (unichar_isdigit(*fmt)) { cnt = get_fmt_num(&fmt); } @@ -208,8 +213,7 @@ STATIC void struct_pack_into_internal(mp_obj_t fmt_in, byte *p, size_t n_args, c memset(p + to_copy, 0, cnt - to_copy); p += cnt; } else { - // If we run out of args then we just finish; CPython would raise struct.error - while (cnt-- && i < n_args) { + while (cnt--) { mp_binary_set_val(fmt_type, *fmt, args[i], &p); // Pad bytes don't have a corresponding argument. if (*fmt != 'x') { @@ -222,7 +226,6 @@ STATIC void struct_pack_into_internal(mp_obj_t fmt_in, byte *p, size_t n_args, c } STATIC mp_obj_t struct_pack(size_t n_args, const mp_obj_t *args) { - // TODO: "The arguments must match the values required by the format exactly." mp_int_t size = MP_OBJ_SMALL_INT_VALUE(struct_calcsize(args[0])); vstr_t vstr; vstr_init_len(&vstr, size); diff --git a/py/objarray.c b/py/objarray.c index 5d83f06977..e0b4cbd55f 100644 --- a/py/objarray.c +++ b/py/objarray.c @@ -97,6 +97,9 @@ STATIC void array_print(const mp_print_t *print, mp_obj_t o_in, mp_print_kind_t #if MICROPY_PY_BUILTINS_BYTEARRAY || MICROPY_PY_ARRAY STATIC mp_obj_array_t *array_new(char typecode, size_t n) { + if (typecode == 'x') { + mp_raise_ValueError(translate("bad typecode")); + } int typecode_size = mp_binary_get_size('@', typecode, NULL); mp_obj_array_t *o = m_new_obj(mp_obj_array_t); #if MICROPY_PY_BUILTINS_BYTEARRAY && MICROPY_PY_ARRAY @@ -126,8 +129,10 @@ STATIC mp_obj_t array_construct(char typecode, mp_obj_t initializer) { || (MICROPY_PY_BUILTINS_BYTEARRAY && MP_OBJ_IS_TYPE(initializer, &mp_type_bytearray))))) && mp_get_buffer(initializer, &bufinfo, MP_BUFFER_READ)) { // construct array from raw bytes - // we round-down the len to make it a multiple of sz (CPython raises error) size_t sz = mp_binary_get_size('@', typecode, NULL); + if (bufinfo.len % sz) { + mp_raise_ValueError(translate("bytes length not a multiple of item size")); + } size_t len = bufinfo.len / sz; mp_obj_array_t *o = array_new(typecode, len); memcpy(o->items, bufinfo.buf, len * sz); diff --git a/tests/basics/struct1.py b/tests/basics/struct1.py index db34342a17..107006cc3f 100644 --- a/tests/basics/struct1.py +++ b/tests/basics/struct1.py @@ -39,6 +39,28 @@ print(v == (10, 100, 200, 300)) # network byte order print(struct.pack('!i', 123)) +# too short / too long arguments +buf = bytearray(b'>>>123<<<') +try: + struct.pack_into('bb', buf, 0, 3) +except: + print('struct.error') + +try: + struct.pack_into('bb', buf, 0, 3, 1, 4) +except: + print('struct.error') + +try: + struct.pack('bb', 3) +except: + print('struct.error') + +try: + struct.pack('bb', 3, 1, 4) +except: + print('struct.error') + # check that we get an error if the buffer is too small try: struct.unpack('I', b'\x00\x00\x00') @@ -96,3 +118,6 @@ try: print(struct.unpack_from(' Date: Sat, 12 Sep 2020 15:11:02 -0500 Subject: [PATCH 2/8] make translate --- locale/circuitpython.pot | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/locale/circuitpython.pot b/locale/circuitpython.pot index af9491c042..78d1223a63 100644 --- a/locale/circuitpython.pot +++ b/locale/circuitpython.pot @@ -8,7 +8,7 @@ msgid "" msgstr "" "Project-Id-Version: PACKAGE VERSION\n" "Report-Msgid-Bugs-To: \n" -"POT-Creation-Date: 2020-09-09 14:33-0700\n" +"POT-Creation-Date: 2020-09-12 15:10-0500\n" "PO-Revision-Date: YEAR-MO-DA HO:MI+ZONE\n" "Last-Translator: FULL NAME \n" "Language-Team: LANGUAGE \n" @@ -1925,7 +1925,7 @@ msgstr "" msgid "bad format string" msgstr "" -#: py/binary.c +#: py/binary.c py/objarray.c msgid "bad typecode" msgstr "" @@ -1978,6 +1978,10 @@ msgstr "" msgid "bytes > 8 bits not supported" msgstr "" +#: py/objarray.c +msgid "bytes length not a multiple of item size" +msgstr "" + #: py/objstr.c msgid "bytes value out of range" msgstr "" @@ -2943,6 +2947,11 @@ msgstr "" msgid "overflow converting long int to machine word" msgstr "" +#: py/modstruct.c +#, c-format +msgid "pack expected %d items for packing (got %d)" +msgstr "" + #: shared-bindings/_stage/Layer.c shared-bindings/_stage/Text.c msgid "palette must be 32 bytes long" msgstr "" From 12d826d941f705fbfa0ced667d1c7efb264d95b1 Mon Sep 17 00:00:00 2001 From: Jeff Epler Date: Sat, 12 Sep 2020 14:14:30 -0500 Subject: [PATCH 3/8] Add FALLTHROUGH comments as needed I investigated these cases and confirmed that the fallthrough behavior was intentional. --- extmod/re1.5/recursiveloop.c | 1 + py/objset.c | 1 + 2 files changed, 2 insertions(+) diff --git a/extmod/re1.5/recursiveloop.c b/extmod/re1.5/recursiveloop.c index bb337decfb..5c1f37a5a1 100644 --- a/extmod/re1.5/recursiveloop.c +++ b/extmod/re1.5/recursiveloop.c @@ -22,6 +22,7 @@ recursiveloop(char *pc, const char *sp, Subject *input, const char **subp, int n case Char: if(*sp != *pc++) return 0; + /* FALLTHROUGH */ case Any: sp++; continue; diff --git a/py/objset.c b/py/objset.c index b8f0f07ede..45b5c12606 100644 --- a/py/objset.c +++ b/py/objset.c @@ -450,6 +450,7 @@ STATIC mp_obj_t set_unary_op(mp_unary_op_t op, mp_obj_t self_in) { return MP_OBJ_NEW_SMALL_INT(hash); } #endif + /* FALLTHROUGH */ default: return MP_OBJ_NULL; // op not supported } } From 5729097bc415a935b0e18b034a6313cf0b74daab Mon Sep 17 00:00:00 2001 From: Jeff Epler Date: Sat, 12 Sep 2020 14:15:32 -0500 Subject: [PATCH 4/8] atmel-samd: enable build error for implicit fallthrough If any diagnostics occur, we will want to either add `/* FALLTHROUGH */` or `break;` as appropriate. I only tested a few builds (trinket m0 and metro m4 express) --- ports/atmel-samd/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ports/atmel-samd/Makefile b/ports/atmel-samd/Makefile index 489f3a7afb..f00366c272 100644 --- a/ports/atmel-samd/Makefile +++ b/ports/atmel-samd/Makefile @@ -148,7 +148,7 @@ else endif endif -CFLAGS += $(INC) -Wall -Werror -std=gnu11 -nostdlib -fshort-enums $(BASE_CFLAGS) $(CFLAGS_MOD) $(COPT) +CFLAGS += $(INC) -Wall -Werror -std=gnu11 -nostdlib -fshort-enums $(BASE_CFLAGS) $(CFLAGS_MOD) $(COPT) -Wimplicit-fallthrough=2 ifeq ($(CHIP_FAMILY), samd21) CFLAGS += \ From 7151ee85ce55683b95bdd19d87e08ed23c857bc6 Mon Sep 17 00:00:00 2001 From: Jeff Epler Date: Sat, 12 Sep 2020 16:30:12 -0500 Subject: [PATCH 5/8] _bleio: add missing break statement --- devices/ble_hci/common-hal/_bleio/att.c | 1 + 1 file changed, 1 insertion(+) diff --git a/devices/ble_hci/common-hal/_bleio/att.c b/devices/ble_hci/common-hal/_bleio/att.c index 591e508a65..6528361cb1 100644 --- a/devices/ble_hci/common-hal/_bleio/att.c +++ b/devices/ble_hci/common-hal/_bleio/att.c @@ -1748,6 +1748,7 @@ void check_att_err(uint8_t err) { break; case BT_ATT_ERR_ENCRYPTION_KEY_SIZE: msg = translate("Encryption key size"); + break; case BT_ATT_ERR_INVALID_ATTRIBUTE_LEN: msg = translate("Invalid attribute length"); break; From 01fdd9598a83fee98b2447c50ea911d7bfe205cb Mon Sep 17 00:00:00 2001 From: Jeff Epler Date: Sat, 12 Sep 2020 17:48:42 -0500 Subject: [PATCH 6/8] disable iimplicit-fallthrough warning inside asf4 .. there is an instance of it that looks like a "true positive", but it only affects sdhc transfers that are not a multiple of 4 bytes, which I don't think happens. (sd card blocks are always 512 bytes) I can fix this in our asf4 repo but that would mean this should be deferred until after #3384 is merged, since that also touches asf4 very invasively by adding a whole new chip family. --- ports/atmel-samd/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ports/atmel-samd/Makefile b/ports/atmel-samd/Makefile index f00366c272..20c79de8f1 100644 --- a/ports/atmel-samd/Makefile +++ b/ports/atmel-samd/Makefile @@ -255,7 +255,7 @@ SRC_ASF += \ hal/src/hal_mci_sync.c \ hpl/sdhc/hpl_sdhc.c \ -$(BUILD)/asf4/$(CHIP_FAMILY)/hpl/sdhc/hpl_sdhc.o: CFLAGS += -Wno-cast-align +$(BUILD)/asf4/$(CHIP_FAMILY)/hpl/sdhc/hpl_sdhc.o: CFLAGS += -Wno-cast-align -Wno-implicit-fallthrough endif $(BUILD)/asf4/$(CHIP_FAMILY)/hpl/sercom/hpl_sercom.o: CFLAGS += -Wno-maybe-uninitialized From 90f7340bfc35d19fcf5e11c078c7f45399ed07e7 Mon Sep 17 00:00:00 2001 From: Jeff Epler Date: Sun, 13 Sep 2020 13:12:35 -0500 Subject: [PATCH 7/8] move implicit-fallthrough warning enable to defns.mk --- ports/atmel-samd/Makefile | 2 +- py/circuitpy_defns.mk | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/ports/atmel-samd/Makefile b/ports/atmel-samd/Makefile index 20c79de8f1..72c7d96882 100644 --- a/ports/atmel-samd/Makefile +++ b/ports/atmel-samd/Makefile @@ -148,7 +148,7 @@ else endif endif -CFLAGS += $(INC) -Wall -Werror -std=gnu11 -nostdlib -fshort-enums $(BASE_CFLAGS) $(CFLAGS_MOD) $(COPT) -Wimplicit-fallthrough=2 +CFLAGS += $(INC) -Wall -Werror -std=gnu11 -nostdlib -fshort-enums $(BASE_CFLAGS) $(CFLAGS_MOD) $(COPT) ifeq ($(CHIP_FAMILY), samd21) CFLAGS += \ diff --git a/py/circuitpy_defns.mk b/py/circuitpy_defns.mk index 04e38d81ea..0e87287c13 100644 --- a/py/circuitpy_defns.mk +++ b/py/circuitpy_defns.mk @@ -31,6 +31,7 @@ BASE_CFLAGS = \ -fsingle-precision-constant \ -fno-strict-aliasing \ -Wdouble-promotion \ + -Wimplicit-fallthrough=2 \ -Wno-endif-labels \ -Wstrict-prototypes \ -Werror-implicit-function-declaration \ From b3bdd4686bd5f4a7c91972b9b0f65e624a7eab95 Mon Sep 17 00:00:00 2001 From: Jeff Epler Date: Sun, 13 Sep 2020 15:10:04 -0500 Subject: [PATCH 8/8] PacketBuffer: add missing 'break's, remove unneeded {} --- ports/nrf/common-hal/_bleio/PacketBuffer.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/ports/nrf/common-hal/_bleio/PacketBuffer.c b/ports/nrf/common-hal/_bleio/PacketBuffer.c index a8773f961f..6d587984ca 100644 --- a/ports/nrf/common-hal/_bleio/PacketBuffer.c +++ b/ports/nrf/common-hal/_bleio/PacketBuffer.c @@ -129,14 +129,12 @@ STATIC bool packet_buffer_on_ble_client_evt(ble_evt_t *ble_evt, void *param) { } break; } - case BLE_GATTC_EVT_WRITE_CMD_TX_COMPLETE: { + case BLE_GATTC_EVT_WRITE_CMD_TX_COMPLETE: queue_next_write(self); break; - } - case BLE_GATTC_EVT_WRITE_RSP: { + case BLE_GATTC_EVT_WRITE_RSP: queue_next_write(self); break; - } default: return false; break; @@ -171,14 +169,14 @@ STATIC bool packet_buffer_on_ble_server_evt(ble_evt_t *ble_evt, void *param) { } break; } - case BLE_GAP_EVT_DISCONNECTED: { + case BLE_GAP_EVT_DISCONNECTED: if (self->conn_handle == ble_evt->evt.gap_evt.conn_handle) { self->conn_handle = BLE_CONN_HANDLE_INVALID; } - } - case BLE_GATTS_EVT_HVN_TX_COMPLETE: { + break; + case BLE_GATTS_EVT_HVN_TX_COMPLETE: queue_next_write(self); - } + break; default: return false; break;