various: Fix gcc -fsanitize=undefined errors #7237

jepler · 2021-05-10T02:47:20Z

../../extmod/crypto-algorithms/sha256.c:49:19: runtime error: left shift of # by 24 places cannot be represented in type 'int'
../../extmod/moduasyncio.c:106:35: runtime error: member access within null pointer of type 'struct mp_obj_task_t'
../../py/binary.c:210:13: runtime error: left shift of negative value -1
../../py/mpz.c:711:5: runtime error: null pointer passed as argument 2, which is declared to never be null
../../py/mpz.c:744:16: runtime error: negation of -9223372036854775808 cannot be represented in type 'long long int'; cast to an unsigned type to negate this value to itself
../../py/objarray.c:130:9: runtime error: null pointer passed as argument 1, which is declared to never be null
../../py/objarray.c:377:5: runtime error: null pointer passed as argument 1, which is declared to never be null
../../py/objarray.c:457:21: runtime error: null pointer passed as argument 1, which is declared to never be null
../../py/objarray.c:457:21: runtime error: null pointer passed as argument 2, which is declared to never be null
../../py/objarray.c:461:21: runtime error: null pointer passed as argument 1, which is declared to never be null
../../py/objdict.c:250:5: runtime error: null pointer passed as argument 1, which is declared to never be null
../../py/objdict.c:250:5: runtime error: null pointer passed as argument 2, which is declared to never be null
../../py/objint.c:109:22: runtime error: left shift of 1 by 31 places cannot be represented in type 'int'
../../py/objint_mpz.c:374:9: runtime error: left shift of 4611686018427387904 by 1 places cannot be represented in type 'long int'
../../py/objint_mpz.c:374:9: runtime error: left shift of negative value -#
../../py/objset.c:181:5: runtime error: null pointer passed as argument 1, which is declared to never be null
../../py/objset.c:181:5: runtime error: null pointer passed as argument 2, which is declared to never be null
../../py/parsenum.c:106:14: runtime error: left shift of 4611686018427387904 by 1 places cannot be represented in type 'long int'
../../py/qstr.c:181:78: runtime error: null pointer passed as argument 2, which is declared to never be null
../../py/runtime.c:395:33: runtime error: left shift of negative value -#
../../py/sequence.c:108:15: runtime error: null pointer passed as argument 1, which is declared to never be null
../../py/sequence.c:108:15: runtime error: null pointer passed as argument 2, which is declared to never be null
../../py/showbc.c:177:28: runtime error: left shift of negative value -1
../../py/vm.c:321:36: runtime error: left shift of negative value -1

With these cumulative changes, make VARIANT=coverage test_full with -fsanitize=undefined passes, except for some problems in axtls.

My testing was done on an amd64 debian buster system using gcc-8.3 and these settings:

CFLAGS += -g3 -Og -fsanitize=undefined
LDFLAGS += -fsanitize=undefined

The changes are intended/expected to produce no runtime overhead, as the behavior of the mem* functions is only actually modified for the unix coverage run (MICROPY_NONNULL_COMPLIANT).

The introduced PAIRHEAP macro's conditional (x ? &x->i : NULL) assembles (under amd64 gcc 8.3 -Os) to the same as &x->i, since i is the initial field of the struct. However, for the purposes of undefined behavior analysis the conditional is needed.

Signed-off-by: Jeff Epler jepler@gmail.com

jepler · 2021-05-10T03:05:07Z

I'll try to sort out the small ARM size difference and any other problems soon.

dpgeorge · 2021-05-10T03:09:30Z

With these cumulative changes, make VARIANT=coverage test_full with -fsanitize=undefined passes, except for some problems in axtls.

Might make sense to always build the coverage variant using fsanitize to make sure these fixes don't regress.

stinos · 2021-05-10T06:50:29Z

py/obj.h


 // Note: dest and slice regions may overlap
 #define mp_seq_replace_slice_no_grow(dest, dest_len, beg, end, slice, slice_len, item_sz) \
-    memmove(((char *)dest) + (beg) * (item_sz), slice, slice_len * (item_sz)); \
-    memmove(((char *)dest) + (beg + slice_len) * (item_sz), ((char *)dest) + (end) * (item_sz), (dest_len - end) * (item_sz));
+    (void)memmove0(((char *)dest) + (beg) * (item_sz), slice, slice_len * (item_sz)); \


Is the (void) suppressing some warning here, or just to inicate the return value is unused?

To mark it as intentionally unused. Without the cast, gcc encounters these diagnostics:

../../py/objlist.c: In function ‘list_subscr’: ../../py/misc.h:335:72: error: statement with no effect [-Werror=unused-value] #define memmove0(dest, src, n) ((n) != 0 ? memmove((dest), (src), (n)) : (dest)) ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~ ../../py/obj.h:1009:5: note: in expansion of macro ‘memmove0’ memmove0(((char *)dest) + (beg) * (item_sz), slice, slice_len * (item_sz)); \ ^~~~~~~~ ../../py/objlist.c:169:13: note: in expansion of macro ‘mp_seq_replace_slice_no_grow’ mp_seq_replace_slice_no_grow(self->items, self->len, slice.start, slice.stop, self->items /*NULL*/, 0, sizeof(*self->items)); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ cc1: all warnings being treated as errors

Ah, right, because it's not a single function call anymore. Makes sense.

stinos · 2021-05-10T06:50:51Z

py/objdict.c

@@ -247,7 +247,9 @@ mp_obj_t mp_obj_dict_copy(mp_obj_t self_in) {
    other->map.all_keys_are_qstrs = self->map.all_keys_are_qstrs;
    other->map.is_fixed = 0;
    other->map.is_ordered = self->map.is_ordered;
-    memcpy(other->map.table, self->map.table, self->map.alloc * sizeof(mp_map_elem_t));
+    if (self->map.alloc) {


Why not memcpy0 ?

Thanks. I originally introduced these conditionals everywhere, before adopting the approach of providing wrapping functions I later adopted, and overlooked (at least) two of them in my own pre-push review. Will fix.

stinos · 2021-05-10T06:52:33Z

Can you fix the linenumbers in the message so they math with the last commit before yours?

[messages truncated to pass ci_commit_formatting_run] [the line numbers match 0e87459] ``` binary.c:210:13: runtime error: left shift of negative val... crypto-algorithms/sha256.c:49:19: runtime error: left shif... moduasyncio.c:106:35: runtime error: member access within ... mpz.c:711:5: runtime error: null pointer passed as argumen... mpz.c:744:16: runtime error: negation of -9223372036854775... objarray.c:130:9: runtime error: null pointer passed as ar... objarray.c:377:5: runtime error: null pointer passed as ar... objarray.c:457:21: runtime error: null pointer passed as a... objarray.c:457:21: runtime error: null pointer passed as a... objarray.c:461:21: runtime error: null pointer passed as a... objdict.c:250:5: runtime error: null pointer passed as arg... objdict.c:250:5: runtime error: null pointer passed as arg... objint.c:109:22: runtime error: left shift of # by 31 plac... objint_mpz.c:374:9: runtime error: left shift of # by 1 pl... objint_mpz.c:374:9: runtime error: left shift of negative ... objset.c:181:5: runtime error: null pointer passed as argu... objset.c:181:5: runtime error: null pointer passed as argu... parsenum.c:106:14: runtime error: left shift of # by 1 pla... qstr.c:181:78: runtime error: null pointer passed as argum... runtime.c:395:33: runtime error: left shift of negative va... runtime.c:534:17: runtime error: left shift of negative va... sequence.c:108:15: runtime error: null pointer passed as a... sequence.c:108:15: runtime error: null pointer passed as a... showbc.c:177:28: runtime error: left shift of negative val... vm.c:321:36: runtime error: left shift of negative value -... ``` With these cumulative changes, `make VARIANT=coverage test_full` with -fsanitize=undefined passes, except for some problems in axtls. My testing was done on an amd64 debian buster system using gcc-8.3 and these settings: ``` CFLAGS += -g3 -Og -fsanitize=undefined LDFLAGS += -fsanitize=undefined ``` The changes are intended/expected to produce no runtime overhead, as the behavior of the mem* functions is only actually modified for the unix coverage run (MICROPY_NONNULL_COMPLIANT). The introduced PAIRHEAP macro's conditional (x ? &x->i : NULL) assembles (under amd64 gcc 8.3 -Os) to the same as &x->i, since i is the initial field of the struct. However, for the purposes of undefined behavior analysis the conditional is needed. Signed-off-by: Jeff Epler <jepler@gmail.com>

jepler · 2021-05-10T19:36:46Z

Since there are remaining sanitizer problems in axtls, those would need to be resolved before turning on the sanitizer in the coverage build. I did file a PR upstream with axtls but I don't know whether this is likely to be acted on: pfalcon/axtls#6 (a possible alternative would be using a suppressions file to suppress these diagnostics)

I've updated the locations both in the commit message and in the initial commit on this PR.

jepler · 2021-05-10T20:00:17Z

The patch in this gist will unconditionally enable the "undefined behavior" sanitizer for coverage builds, while excluding axtls files from the sanitizer: https://gist.github.com/bc669a1f68d413afa2b9c1b14fd9956b

Some reasons not to do this would include:

there's also the "address sanitizer", which is mutually exclusive, and may also be of interest (so should these be additional variants?)
I'm not happy about the idea of excluding code, even if it's external code, though the flaws that ubsan found are not likely to cause problems (none of these are likely to cause problems, they're just standards-compliance)

dpgeorge · 2021-05-30T02:53:46Z

py/objset.c

@@ -178,7 +178,9 @@ STATIC mp_obj_t set_copy(mp_obj_t self_in) {
    other->base.type = self->base.type;
    mp_set_init(&other->set, self->set.alloc);
    other->set.used = self->set.used;
-    memcpy(other->set.table, self->set.table, self->set.alloc * sizeof(mp_obj_t));
+    if (self->set.alloc) {
+        memcpy(other->set.table, self->set.table, self->set.alloc * sizeof(mp_obj_t));


can this use memcpy0?

dpgeorge · 2021-05-30T02:55:06Z

It would be great to separate this PR into 2 commits:

memcpy0 etc change: adding of the MICROPY_NONNULL_COMPLIANT option, the memcpy0 etc macros, and changing various locations to use it
remaining changes

dpgeorge · 2021-05-30T02:57:36Z

there's also the "address sanitizer", which is mutually exclusive, and may also be of interest (so should these be additional variants?)

Ok, yes, makes sense to have additional builds then, one for -fsanitize-undefined and one for -fsanitize=address (can be done separate to this PR).

I'm not happy about the idea of excluding code, even if it's external code

We can fork axtls and/or berkeley repos to fix the issues with them (there are other things that would be good to fix in those libraries as well).

jimmo · 2022-11-15T00:59:23Z

Thanks for splitting the PRs @jepler. Closing this PR in favour of #7369 (still pending) and #7370 (already merged).

Add Boardsource Blok

jepler force-pushed the sanitize-undefined-fixes branch 2 times, most recently from ab63ea5 to 32f2e9f Compare May 10, 2021 03:02

dpgeorge added the py-core label May 10, 2021

stinos reviewed May 10, 2021

View reviewed changes

jepler force-pushed the sanitize-undefined-fixes branch from 32f2e9f to 29cd97e Compare May 10, 2021 13:00

jepler force-pushed the sanitize-undefined-fixes branch from 29cd97e to cc74f22 Compare May 10, 2021 13:38

dpgeorge reviewed May 30, 2021

View reviewed changes

jepler mentioned this pull request Jun 8, 2021

various: Fix null pointer errors #7369

Open

jepler mentioned this pull request Nov 21, 2021

various: Fix -fsanitize=undefined errors. #7370

Closed

jimmo closed this Nov 15, 2022

jepler mentioned this pull request Nov 15, 2022

signed integer overflow (undefined behavior) in littlefs #9957

Open

tannewt pushed a commit to tannewt/circuitpython that referenced this pull request Mar 21, 2023

Merge pull request micropython#7237 from kdb424/main

37b9f77

Add Boardsource Blok

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

various: Fix gcc -fsanitize=undefined errors #7237

various: Fix gcc -fsanitize=undefined errors #7237

jepler commented May 10, 2021 •

edited

jepler commented May 10, 2021

dpgeorge commented May 10, 2021

stinos May 10, 2021

jepler May 10, 2021

stinos May 10, 2021

stinos May 10, 2021

jepler May 10, 2021

stinos commented May 10, 2021

jepler commented May 10, 2021

jepler commented May 10, 2021

dpgeorge May 30, 2021

dpgeorge commented May 30, 2021

dpgeorge commented May 30, 2021

jimmo commented Nov 15, 2022

various: Fix gcc -fsanitize=undefined errors #7237

various: Fix gcc -fsanitize=undefined errors #7237

Conversation

jepler commented May 10, 2021 • edited

jepler commented May 10, 2021

dpgeorge commented May 10, 2021

stinos May 10, 2021

Choose a reason for hiding this comment

jepler May 10, 2021

Choose a reason for hiding this comment

stinos May 10, 2021

Choose a reason for hiding this comment

stinos May 10, 2021

Choose a reason for hiding this comment

jepler May 10, 2021

Choose a reason for hiding this comment

stinos commented May 10, 2021

jepler commented May 10, 2021

jepler commented May 10, 2021

dpgeorge May 30, 2021

Choose a reason for hiding this comment

dpgeorge commented May 30, 2021

dpgeorge commented May 30, 2021

jimmo commented Nov 15, 2022

jepler commented May 10, 2021 •

edited