Hello Nathan,
21.08.2024 00:21, Nathan Bossart wrote:
I've combined all the current proposed changes into one patch. I've also
introduced signed versions of the negation functions into int.h to avoid
relying on multiplication.
Thank you for taking care of this!
I'd like to add some info to show how big the iceberg is.
Beside other trap-triggered places in date/time conversion functions, I
also discovered:
1)
CREATE TABLE jt(j jsonb); INSERT INTO jt VALUES('[]'::jsonb);
UPDATE jt SET j[0][-2147483648] = '0';
#4 0x00007f15ab00d7f3 in __GI_abort () at ./stdlib/abort.c:79
#5 0x00005570113b2591 in __addvsi3 ()
#6 0x00005570111d55a0 in push_null_elements (ps=0x7fff37385fb8,
num=-2147483648) at jsonfuncs.c:1707
#7 0x00005570111d5749 in push_path (st=0x7fff37385fb8, level=0,
path_elems=0x55701300c880, path_nulls=0x55701300d520,
path_len=2, newval=0x7fff37386030) at jsonfuncs.c:1770
The "problematic" code:
while (num-- > 0)
*ps = 0;
looks innocent to me, but is not for good enough for -ftrapv.
I think there could be other similar places and this raises two questions:
can they be reached with INT_MIN and what to do if so?
By the way, the same can be seen with CC=clang CPPFLAGS="-ftrapv". Please
look at the code produced by both compilers for x86_64:
https://godbolt.org/z/vjszjf4b3
(clang generates ud1, while gcc uses call __addvsi3)
The aside question is: should jsonb subscripting accept negative indexes
when the target array is not initialized yet?
Compare:
CREATE TABLE jt(j jsonb); INSERT INTO jt VALUES('[]'::jsonb);
UPDATE jt SET j[0][-1] = '0';
SELECT * FROM jt;
j
-------
[[0]]
with
CREATE TABLE jt(j jsonb); INSERT INTO jt VALUES('[[]]'::jsonb);
UPDATE jt SET j[0][-1] = '0';
ERROR: path element at position 2 is out of range: -1
2)
SELECT x, lag(x, -2147483648) OVER (ORDER BY x) FROM (SELECT 1) x;
#4 0x00007fa7d00f47f3 in __GI_abort () at ./stdlib/abort.c:79
#5 0x00005623a7336851 in __negvsi2 ()
#6 0x00005623a726ae35 in leadlag_common (fcinfo=0x7ffd59cca950, forward=false,
withoffset=true, withdefault=false)
at windowfuncs.c:551
#7 0x00005623a726af19 in window_lag_with_offset (fcinfo=0x7ffd59cca950) at
windowfuncs.c:594
As to 32-bit Debian, I wrote about before, I use gcc (Debian 12.2.0-14).
Please look at the demo code (and it's assembly, produced with
gcc -S -ftrapv t.c) attached:
gcc -Wall -Wextra -fsanitize=signed-integer-overflow -Wstrict-overflow=5 \
-O0 -ftrapv t.c -o t && ./t
Aborted (core dumped)
#4 0xb762226a in __GI_abort () at ./stdlib/abort.c:79
#5 0x00495077 in __mulvdi3.cold ()
#6 0x00495347 in pg_mul_s64_overflow ()
(It looks like -Wstrict-overflow can't help with the static analysis
desired in such cases.)
Moreover, I got `make check` failed with -ftrapv on aarch64 (using gcc 8.3)
as follows:
#1 0x0000007e1edc48e8 in __GI_abort () at abort.c:79
#2 0x0000005ee66b71cc in __subvdi3 ()
#3 0x0000005ee6560e24 in int8gcd_internal (arg1=-9223372036854775808, arg2=1)
at int8.c:623
#4 0x0000005ee62f576c in ExecInterpExpr (state=0x5eeaba9d18, econtext=0x5eeaba95f0,
isnull=<optimized out>)
at execExprInterp.c:770
...
#13 0x0000005ee64e5d84 in exec_simple_query (
query_string=query_string@entry=0x5eeaac7500 "SELECT a, b, gcd(a, b), gcd(a, -b), gcd(b, a), gcd(-b, a)\nFROM
(VALUES (0::int8, 0::int8),\n", ' ' <repeats 13 times>, "(0::int8, 29893644334::int8),\n", ' ' <repeats 13 times>,
"(288484263558::int8, 29893644334::int8),\n", ' ' <repeats 12 times>...) at postgres.c:1284
So I wonder whether enabling -ftrapv can really help us prepare the code
for -fno-wrapv?
Best regards,
Alexander
#include <stdio.h>
typedef char bool;
typedef long long int int64;
static inline bool
pg_mul_s64_overflow(int64 a, int64 b, int64 *result)
{
return __builtin_mul_overflow(a, b, result);
}
int main()
{
int64 a = 9223372036854775807L;
int64 b = 2L;
int64 c = 0;
bool r;
r = pg_mul_s64_overflow(a, b, &c);
printf("r: %d, c: %lld\n", r, c);
}
.file "t.c"
.text
.globl __mulvdi3
.type pg_mul_s64_overflow, @function
pg_mul_s64_overflow:
.LFB0:
.cfi_startproc
pushl %ebp
.cfi_def_cfa_offset 8
.cfi_offset 5, -8
movl %esp, %ebp
.cfi_def_cfa_register 5
pushl %edi
pushl %esi
pushl %ebx
subl $76, %esp
.cfi_offset 7, -12
.cfi_offset 6, -16
.cfi_offset 3, -20
call __x86.get_pc_thunk.ax
addl $_GLOBAL_OFFSET_TABLE_, %eax
movl %eax, -52(%ebp)
movl 8(%ebp), %eax
movl %eax, -32(%ebp)
movl 12(%ebp), %eax
movl %eax, -28(%ebp)
movl 16(%ebp), %eax
movl %eax, -40(%ebp)
movl 20(%ebp), %eax
movl %eax, -36(%ebp)
movl $0, -64(%ebp)
movl $0, -60(%ebp)
movl -32(%ebp), %eax
movl -28(%ebp), %edx
movl %edx, %eax
movl %eax, %edx
sarl $31, %edx
movl %eax, -48(%ebp)
movl %edx, -44(%ebp)
movl -32(%ebp), %eax
movl %eax, %ecx
sarl $31, %ecx
movl -40(%ebp), %eax
movl -36(%ebp), %edx
movl %eax, %esi
movl %edx, %edi
movl %edi, %esi
movl %esi, %edi
sarl $31, %edi
movl -40(%ebp), %eax
sarl $31, %eax
movl -48(%ebp), %ebx
cmpl %ebx, %ecx
jne .L4
cmpl %esi, %eax
jne .L5
movl -40(%ebp), %eax
imull -32(%ebp)
jmp .L2
.L5:
movl -40(%ebp), %eax
movl -36(%ebp), %edx
movl %eax, -72(%ebp)
movl %edx, -68(%ebp)
movl -32(%ebp), %eax
movl %eax, -48(%ebp)
jmp .L6
.L4:
cmpl %esi, %eax
jne .L7
movl -32(%ebp), %eax
movl -28(%ebp), %edx
movl %eax, -72(%ebp)
movl %edx, -68(%ebp)
movl -48(%ebp), %esi
movl -40(%ebp), %eax
movl %eax, -48(%ebp)
.L6:
movl -40(%ebp), %edi
movl %edi, %eax
mull -32(%ebp)
movl %eax, -80(%ebp)
movl %edx, -76(%ebp)
movl -48(%ebp), %edi
movl %edi, %eax
mull %esi
movl %eax, %ecx
movl %edx, %ebx
testl %esi, %esi
jns .L8
movl %edi, %eax
movl $0, %edx
movl %eax, %edx
movl $0, %eax
movl %eax, %esi
movl %edx, %edi
movl %ecx, %eax
movl %ebx, %edx
subl %esi, %eax
sbbl %edi, %edx
movl %eax, %ecx
movl %edx, %ebx
.L8:
cmpl $0, -48(%ebp)
jns .L9
movl %ecx, %eax
movl %ebx, %edx
subl -72(%ebp), %eax
sbbl -68(%ebp), %edx
movl %eax, %ecx
movl %edx, %ebx
.L9:
movl -80(%ebp), %eax
movl -76(%ebp), %edx
movl %edx, %eax
xorl %edx, %edx
addl %ecx, %eax
adcl %ebx, %edx
movl %eax, %ecx
movl %edx, %ebx
movl %ecx, %eax
movl %ebx, %edx
movl %edx, %eax
movl %eax, %edx
sarl $31, %edx
movl %ecx, %esi
sarl $31, %esi
cmpl %eax, %esi
jne .L10
movl %ecx, %ebx
movl $0, %ecx
movl %ecx, %eax
movl %ebx, %edx
movl -80(%ebp), %ecx
movl $0, %ebx
orl %ecx, %eax
orl %ebx, %edx
jmp .L2
.L7:
pushl -36(%ebp)
pushl -40(%ebp)
pushl -28(%ebp)
pushl -32(%ebp)
movl -52(%ebp), %ebx
call __mulvdi3@PLT
addl $16, %esp
movl -48(%ebp), %ebx
leal 1(%ebx), %ecx
cmpl $1, %ecx
ja .L3
leal 1(%esi), %ecx
cmpl $1, %ecx
ja .L3
cmpl %esi, -48(%ebp)
jne .L11
movl $0, %ebx
movl $0, %ecx
cmpl %eax, %ebx
sbbl %edx, %ecx
jge .L3
jmp .L2
.L11:
testl %edx, %edx
jns .L3
jmp .L2
.L10:
pushl -36(%ebp)
pushl -40(%ebp)
pushl -28(%ebp)
pushl -32(%ebp)
movl -52(%ebp), %ebx
call __mulvdi3@PLT
addl $16, %esp
.L3:
movl $1, -64(%ebp)
movl $0, -60(%ebp)
.L2:
movl 24(%ebp), %ecx
movl %eax, (%ecx)
movl %edx, 4(%ecx)
movl -64(%ebp), %eax
movl -60(%ebp), %edx
andl $1, %eax
leal -12(%ebp), %esp
popl %ebx
.cfi_restore 3
popl %esi
.cfi_restore 6
popl %edi
.cfi_restore 7
popl %ebp
.cfi_restore 5
.cfi_def_cfa 4, 4
ret
.cfi_endproc
.LFE0:
.size pg_mul_s64_overflow, .-pg_mul_s64_overflow
.section .rodata
.LC0:
.string "r: %d, c: %lld\n"
.text
.globl main
.type main, @function
main:
.LFB1:
.cfi_startproc
leal 4(%esp), %ecx
.cfi_def_cfa 1, 0
andl $-16, %esp
pushl -4(%ecx)
pushl %ebp
movl %esp, %ebp
.cfi_escape 0x10,0x5,0x2,0x75,0
pushl %ebx
pushl %ecx
.cfi_escape 0xf,0x3,0x75,0x78,0x6
.cfi_escape 0x10,0x3,0x2,0x75,0x7c
subl $32, %esp
call __x86.get_pc_thunk.bx
addl $_GLOBAL_OFFSET_TABLE_, %ebx
movl $-1, -16(%ebp)
movl $2147483647, -12(%ebp)
movl $2, -24(%ebp)
movl $0, -20(%ebp)
movl $0, -40(%ebp)
movl $0, -36(%ebp)
subl $12, %esp
leal -40(%ebp), %eax
pushl %eax
pushl -20(%ebp)
pushl -24(%ebp)
pushl -12(%ebp)
pushl -16(%ebp)
call pg_mul_s64_overflow
addl $32, %esp
movb %al, -25(%ebp)
movl -40(%ebp), %eax
movl -36(%ebp), %edx
movsbl -25(%ebp), %ecx
pushl %edx
pushl %eax
pushl %ecx
leal .LC0@GOTOFF(%ebx), %eax
pushl %eax
call printf@PLT
addl $16, %esp
movl $0, %eax
leal -8(%ebp), %esp
popl %ecx
.cfi_restore 1
.cfi_def_cfa 1, 0
popl %ebx
.cfi_restore 3
popl %ebp
.cfi_restore 5
leal -4(%ecx), %esp
.cfi_def_cfa 4, 4
ret
.cfi_endproc
.LFE1:
.size main, .-main
.section
.text.__x86.get_pc_thunk.ax,"axG",@progbits,__x86.get_pc_thunk.ax,comdat
.globl __x86.get_pc_thunk.ax
.hidden __x86.get_pc_thunk.ax
.type __x86.get_pc_thunk.ax, @function
__x86.get_pc_thunk.ax:
.LFB2:
.cfi_startproc
movl (%esp), %eax
ret
.cfi_endproc
.LFE2:
.section
.text.__x86.get_pc_thunk.bx,"axG",@progbits,__x86.get_pc_thunk.bx,comdat
.globl __x86.get_pc_thunk.bx
.hidden __x86.get_pc_thunk.bx
.type __x86.get_pc_thunk.bx, @function
__x86.get_pc_thunk.bx:
.LFB3:
.cfi_startproc
movl (%esp), %ebx
ret
.cfi_endproc
.LFE3:
.ident "GCC: (Debian 12.2.0-14) 12.2.0"
.section .note.GNU-stack,"",@progbits
