signed integer overflow (undefined behavior) in littlefs
While updating #7237 I encountered the following diagnostic from the gcc-10 undefined behavior sanitizer:
--- /home/jepler/src/micropython/tests/results/extmod_vfs_lfs_error.py.exp 2022-11-14 19:56:52.599698807 -0600
+++ /home/jepler/src/micropython/tests/results/extmod_vfs_lfs_error.py.out 2022-11-14 19:56:52.599698807 -0600
@@ -25,4 +25,5 @@
chdir OSError
/
stat OSError
+../../lib/littlefs/lfs2.c:3461:36: runtime error: signed integer overflow: 1073741824 + 1073741824 cannot be represented in type 'int'
seek OSError
FAILURE /home/jepler/src/micropython/tests/results/extmod_vfs_lfs_error.py
By performing the arithmetic as unsigned, then converting to signed for the comparison, the diagnostic goes away. However, I'm not confident this is an appropriate or complete fix for the problem of seek arithmetic in lfs2_file_rawseek, so I didn't file a PR:
diff --git a/lib/littlefs/lfs2.c b/lib/littlefs/lfs2.c
index a9fcffafd..d3795fec8 100644
--- a/lib/littlefs/lfs2.c
+++ b/lib/littlefs/lfs2.c
@@ -3458,10 +3458,10 @@ static lfs2_soff_t lfs2_file_rawseek(lfs2_t *lfs2, lfs2_file_t *file,
if (whence == LFS2_SEEK_SET) {
npos = off;
} else if (whence == LFS2_SEEK_CUR) {
- if ((lfs2_soff_t)file->pos + off < 0) {
+ npos = file->pos + off;
+ if ((lfs2_soff_t)npos < 0) {
return LFS2_ERR_INVAL;
} else {
- npos = file->pos + off;
}
} else if (whence == LFS2_SEEK_END) {
lfs2_soff_t res = lfs2_file_rawsize(lfs2, file) + off;
longlong: Add sanitizer builds, fix revealed problems.
Summary
I decided to try the longlong build under the sanitizers. This exposed a pair of problems:
- A line, already commented to explain undefined integer overflow behavior, did in fact produce a diagnostic
- The address sanitizer produced spurious results on 32-bit x86 builds when raising exceptions.
Testing
I built the branch locally with the sanitizers until it passed.
Trade-offs and Alternatives
It's odd that the fixed version of left shift doesn't produce a diagnostic, because there is still an overflowing operation (now a multiplication, previously a shift). Is the sanitizer deliberately not checking this kind of overflow, or is recognizing a specific overflow check pattern in the following line? I was not able to find any documentation about this, and the documentation seemed to imply that -fsanitize=signed-integer-overflow was enabled by -fsanitize=undefined; explicitly enabling it made no difference.
It's unclear why adding the returns_twice qualification to nlr_push helps the sanitizer to function properly, but it's true that nlr_push should have this annotation. Hence, maybe it should be enabled whenever the compiler supports it? (any plausible gcc & clang versions, probably)