[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: GNU poke 2.90.0 on Linux/x86_64 with UBsan
|
From: |
Jose E. Marchesi |
|
Subject: |
Re: GNU poke 2.90.0 on Linux/x86_64 with UBsan |
|
Date: |
Mon, 23 Jan 2023 15:57:38 +0100 |
|
User-agent: |
Gnus/5.13 (Gnus v5.13) |
Hi Bruno.
Thanks for the testing and reporting.
> "gcc -ftrapv" checks only for one single case of undefined behaviour,
> namely specific cases of integer overflow.
>
> More generically, one can use clang's UB sanitizer. I installed clang 14.0.0
> then set
>
> CC="clang
> -fsanitize=undefined,signed-integer-overflow,shift,integer-divide-by-zero
> -fno-sanitize=pointer-overflow"
> CFLAGS="-O1 -fno-omit-frame-pointer -ggdb"
> (per recommendations from
> https://blogs.oracle.com/linux/post/improving-application-security-with-undefinedbehaviorsanitizer-ubsan-and-gcc)
>
> then built poke and did "make check". Find attached the poke.log. It has
> several occurrences of
>
> runtime error: left shift of negative value
>
> but also several occurrences of
>
> runtime error: left shift of <X> by <Y> places cannot be represented in
> type 'int32_t'
> runtime error: left shift of <X> by <Y> places cannot be represented in
> type 'int64_t'
> where <X> and <Y> are positive integers.
I installed the two patches below and that made all the UBsan runtime
errors to go away.
There were no regressions, but still I would appreciate if somebody
would take a look at the changes. These are easy to get wrong.
>From f18d5418e89defdb6666bd0ea0f8bdc180ce1ff4 Mon Sep 17 00:00:00 2001
From: "Jose E. Marchesi" <jose.marchesi@oracle.com>
Date: Mon, 23 Jan 2023 15:37:06 +0100
Subject: [PATCH 1/3] ios,pvm: remove UB related to left-shifting
2023-01-23 Jose E. Marchesi <jemarch@gnu.org>
* libpoke/ios.c (ios_read_int): Avoid UB in signed left shifts,
and overflows in unsigned left shifts.
* libpoke/pvm-val.h (PVM_VAL_INT): Likewise.
(PVM_MAKE_INT): Likewise.
(PVM_MAKE_UINT): Likewise.
(PVM_VAL_LONG): Likewise.
* libpoke/pvm.jitter (PVM_ADD_SIGNED): Likewise.
(PVM_SUB_SIGNED): Likewise.
(PVM_MUL_SIGNED): Likewise.
---
ChangeLog | 12 ++++++++++++
libpoke/ios.c | 27 +++++++++++++++------------
libpoke/pvm-val.h | 12 ++++++------
libpoke/pvm.jitter | 10 +++++-----
4 files changed, 38 insertions(+), 23 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index 51739797..49270e37 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,15 @@
+2023-01-23 Jose E. Marchesi <jemarch@gnu.org>
+
+ * libpoke/ios.c (ios_read_int): Avoid UB in signed left shifts,
+ and overflows in unsigned left shifts.
+ * libpoke/pvm-val.h (PVM_VAL_INT): Likewise.
+ (PVM_MAKE_INT): Likewise.
+ (PVM_MAKE_UINT): Likewise.
+ (PVM_VAL_LONG): Likewise.
+ * libpoke/pvm.jitter (PVM_ADD_SIGNED): Likewise.
+ (PVM_SUB_SIGNED): Likewise.
+ (PVM_MUL_SIGNED): Likewise.
+
2023-01-23 Arsen Arsenović <arsen@aarsen.me>
* doc/poke.texi (Exceptions): Fix typo in the E_no_return
diff --git a/libpoke/ios.c b/libpoke/ios.c
index 8e10d3f8..151096d4 100644
--- a/libpoke/ios.c
+++ b/libpoke/ios.c
@@ -760,19 +760,21 @@ ios_read_int (ios io, ios_off offset, int flags,
case 16:
{
if (endian == IOS_ENDIAN_LSB)
- *value = (int16_t) (c[1] << 8) | c[0];
+ *value = ((uint64_t) c[1] << 8) | c[0];
else
- *value = (int16_t) (c[0] << 8) | c[1];
+ *value = ((uint64_t) c[0] << 8) | c[1];
+ *value = (uint64_t) *value << 48;
+ *value >>= 48;
return IOS_OK;
}
case 24:
{
if (endian == IOS_ENDIAN_LSB)
- *value = (c[2] << 16) | (c[1] << 8) | c[0];
+ *value = ((uint64_t) c[2] << 16) | ((uint64_t) c[1] << 8) | c[0];
else
- *value = (c[0] << 16) | (c[1] << 8) | c[2];
- *value <<= 40;
+ *value = ((uint64_t) c[0] << 16) | ((uint64_t) c[1] << 8) | c[2];
+ *value = (uint64_t) *value << 40;
*value >>= 40;
return IOS_OK;
}
@@ -780,9 +782,11 @@ ios_read_int (ios io, ios_off offset, int flags,
case 32:
{
if (endian == IOS_ENDIAN_LSB)
- *value = (int32_t) (c[3] << 24) | (c[2] << 16) | (c[1] << 8) |
c[0];
+ *value = ((uint64_t) c[3] << 24) | ((uint64_t) c[2] << 16) |
((uint64_t) c[1] << 8) | c[0];
else
- *value = (int32_t) (c[0] << 24) | (c[1] << 16) | (c[2] << 8) |
c[3];
+ *value = ((uint64_t) c[0] << 24) | ((uint64_t) c[1] << 16) |
((uint64_t) c[2] << 8) | c[3];
+ *value = (uint64_t) *value << 32;
+ *value >>= 32;
return IOS_OK;
}
@@ -794,7 +798,7 @@ ios_read_int (ios io, ios_off offset, int flags,
else
*value = ((uint64_t) c[0] << 32) | ((uint64_t) c[1] << 24)
| (c[2] << 16) | (c[3] << 8) | c[4];
- *value <<= 24;
+ *value = (uint64_t) *value << 24;
*value >>= 24;
return IOS_OK;
}
@@ -809,7 +813,7 @@ ios_read_int (ios io, ios_off offset, int flags,
*value = ((uint64_t) c[0] << 40) | ((uint64_t) c[1] << 32)
| ((uint64_t) c[2] << 24) | (c[3] << 16)
| (c[4] << 8) | c[5];
- *value <<= 16;
+ *value = (uint64_t) *value << 16;
*value >>= 16;
return IOS_OK;
}
@@ -824,7 +828,7 @@ ios_read_int (ios io, ios_off offset, int flags,
*value = ((uint64_t) c[0] << 48) | ((uint64_t) c[1] << 40)
| ((uint64_t) c[2] << 32) | ((uint64_t) c[3] << 24)
| (c[4] << 16) | (c[5] << 8) | c[6];
- *value <<= 8;
+ *value = (uint64_t) *value << 8;
*value >>= 8;
return IOS_OK;
}
@@ -851,8 +855,7 @@ ios_read_int (ios io, ios_off offset, int flags,
(uint64_t *) value);
if (ret_val == IOS_OK)
{
- *value <<= 64 - bits;
- *value >>= 64 - bits;
+ *value = ((int64_t) (((uint64_t) *value) << (64 - bits))) >> (64 - bits);
return IOS_OK;
}
return ret_val;
diff --git a/libpoke/pvm-val.h b/libpoke/pvm-val.h
index 4be0213f..a98bc5d3 100644
--- a/libpoke/pvm-val.h
+++ b/libpoke/pvm-val.h
@@ -60,12 +60,12 @@
Bits marked with `x' are unused and should be always 0. */
#define PVM_VAL_INT_SIZE(V) (((int) (((V) >> 3) & 0x1f)) + 1)
-#define PVM_VAL_INT(V) (((int32_t) ((V) >> 32)) \
- << (32 - PVM_VAL_INT_SIZE ((V))) \
+#define PVM_VAL_INT(V) (((int32_t) (((uint32_t) ((V) >> 32)) \
+ << (32 - PVM_VAL_INT_SIZE ((V))))) \
>> (32 - PVM_VAL_INT_SIZE ((V))))
#define PVM_MAKE_INT(V,S) \
(((((uint64_t) (int64_t) (V)) & 0xffffffff) << 32) \
- | ((((S) - 1) & 0x1f) << 3) \
+ | ((uint32_t)(((S) - 1) & 0x1f) << 3) \
| PVM_VAL_TAG_INT)
#define PVM_VAL_UINT_SIZE(V) (((int) (((V) >> 3) & 0x1f)) + 1)
@@ -73,7 +73,7 @@
& ((uint32_t) (~( ((~0ul) << ((PVM_VAL_UINT_SIZE
((V)))-1)) << 1 ))))
#define PVM_MAKE_UINT(V,S) \
(((((uint64_t) (V)) & 0xffffffff) << 32) \
- | ((((S) - 1) & 0x1f) << 3) \
+ | ((uint32_t) (((S) - 1) & 0x1f) << 3) \
| PVM_VAL_TAG_UINT)
#define PVM_MAX_UINT(size) ((1U << (size)) - 1)
@@ -108,8 +108,8 @@
((uint64_t) (uintptr_t) ll) | (T); })
#define PVM_VAL_LONG_SIZE(V) (_PVM_VAL_LONG_ULONG_SIZE (V))
-#define PVM_VAL_LONG(V) (_PVM_VAL_LONG_ULONG_VAL ((V)) \
- << (64 - PVM_VAL_LONG_SIZE ((V))) \
+#define PVM_VAL_LONG(V) (((int64_t) ((uint64_t) _PVM_VAL_LONG_ULONG_VAL ((V)) \
+ << (64 - PVM_VAL_LONG_SIZE ((V))))) \
>> (64 - PVM_VAL_LONG_SIZE ((V))))
#define PVM_MAKE_LONG(V,S) \
(PVM_MAKE_LONG_ULONG ((V),(S),PVM_VAL_TAG_LONG))
diff --git a/libpoke/pvm.jitter b/libpoke/pvm.jitter
index 8f7f33aa..9e9e6f2c 100644
--- a/libpoke/pvm.jitter
+++ b/libpoke/pvm.jitter
@@ -344,8 +344,8 @@ late-header-c
CTYPE a = PVM_VAL_##TYPE (JITTER_UNDER_TOP_STACK ()); \
CTYPE b = PVM_VAL_##TYPE (JITTER_TOP_STACK ()); \
int size = PVM_VAL_##TYPE##_SIZE (JITTER_TOP_STACK ()); \
- int64_t a64 = ((int64_t) a << (64 - size)); \
- int64_t b64 = ((int64_t) b << (64 - size)); \
+ int64_t a64 = ((uint64_t)(int64_t) a) << (64 - size); \
+ int64_t b64 = ((uint64_t)(int64_t) b) << (64 - size); \
\
if (INT_ADD_OVERFLOW (a64, b64)) \
PVM_RAISE_DFL (PVM_E_OVERFLOW); \
@@ -364,8 +364,8 @@ late-header-c
CTYPE a = PVM_VAL_##TYPE (JITTER_UNDER_TOP_STACK ()); \
CTYPE b = PVM_VAL_##TYPE (JITTER_TOP_STACK ()); \
int size = PVM_VAL_##TYPE##_SIZE (JITTER_TOP_STACK ()); \
- int64_t a64 = ((int64_t) a << (64 - size)); \
- int64_t b64 = ((int64_t) b << (64 - size)); \
+ int64_t a64 = ((uint64_t)(int64_t) a) << (64 - size); \
+ int64_t b64 = ((uint64_t)(int64_t) b) << (64 - size); \
\
if (INT_SUBTRACT_OVERFLOW (a64, b64)) \
PVM_RAISE_DFL (PVM_E_OVERFLOW); \
@@ -384,7 +384,7 @@ late-header-c
CTYPE a = PVM_VAL_##TYPE (JITTER_UNDER_TOP_STACK ()); \
CTYPE b = PVM_VAL_##TYPE (JITTER_TOP_STACK ()); \
int size = PVM_VAL_##TYPE##_SIZE (JITTER_TOP_STACK ()); \
- int64_t a64 = ((int64_t) a << (64 - size)); \
+ int64_t a64 = ((uint64_t)(int64_t) a) << (64 - size); \
\
if (INT_MULTIPLY_OVERFLOW (a64, b)) \
PVM_RAISE_DFL (PVM_E_OVERFLOW); \
--
2.30.2
>From afe93eb61e436faebd7232d5212e19683faba5ee Mon Sep 17 00:00:00 2001
From: "Jose E. Marchesi" <jose.marchesi@oracle.com>
Date: Mon, 23 Jan 2023 15:54:21 +0100
Subject: [PATCH 2/2] pvm.jitter: avoid left-shit UB in PVM_BINOP_SL
2023-01-23 Jose E. Marchesi <jemarch@gnu.org>
* libpoke/pvm.jitter (PVM_BINOP_SL): Do not trigger left-shit UB.
---
ChangeLog | 4 ++++
libpoke/pvm.jitter | 5 ++++-
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/ChangeLog b/ChangeLog
index d471c624..71785fcd 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+2023-01-23 Jose E. Marchesi <jemarch@gnu.org>
+
+ * libpoke/pvm.jitter (PVM_BINOP_SL): Do not trigger left-shit UB.
+
2023-01-23 Jose E. Marchesi <jemarch@gnu.org>
* libpoke/pkl-fold.c (pkl_phase_fold): Remove duplicated handler
diff --git a/libpoke/pvm.jitter b/libpoke/pvm.jitter
index 9e9e6f2c..96caa3f6 100644
--- a/libpoke/pvm.jitter
+++ b/libpoke/pvm.jitter
@@ -466,7 +466,10 @@ late-header-c
} \
else \
{ \
- PVM_BINOP (TYPEA, TYPEB, TYPER, OP); \
+ int size = PVM_VAL_##TYPER##_SIZE (JITTER_UNDER_TOP_STACK ()); \
+ pvm_val res = PVM_MAKE_##TYPER ((uint64_t) PVM_VAL_##TYPEA
(JITTER_UNDER_TOP_STACK ()) \
+ << PVM_VAL_##TYPEB (JITTER_TOP_STACK
()), size); \
+ JITTER_PUSH_STACK (res); \
} \
}
--
2.30.2