integer overflow in pager.c
hello! it seems like you have an integer overflow in a pager code [right here](https://github.com/sqlite/sqlite/blob/f43fef29bb78dd5194cf39c12e74552f411cd4d0/src/pager.c#L6941) `pRel->iSubRec` have type `Pgno` (which is an alias to `u32`) and `pPager->pageSize+4` have type `int` it may overflow if `pageSize` is maximum (which is 65536 IIRC) and number of subrecords is also somewhere around 65536. also, on [line 3434](https://github.com/sqlite/sqlite/blob/f43fef29bb78dd5194cf39c12e74552f411cd4d0/src/pager.c#L3434) `iSubRec` is casted to `i64` before a similar multiplication, which kinda confirms that it is a bug
(2) By Gunter Hick (gunter_hick) on 2021-09-13 13:10:35 in reply to 1
I don't think so. Both statements multiply two 32 bit integers yielding a 64 bit result. The values you claim as problematic both use 16 or 17 bits, which would overflow a 32bit result, but come nowhere near overflowing 64 bit integers. i64 offset = (i64)pSavepoint->iSubRec*(4+pPager->pageSize); i64 sz = (pPager->pageSize+4)*pRel->iSubRec; Even multiplying the maximum pagesize (17 bits) by maxint (31 bits) only yields 48 bits of product, well within the 63 bits available for positive integers.
`(pPager->pageSize+4)*pRel->iSubRec` has type of `u32` and then is assigned to `i64`
> Both statements multiply two 32 bit integers yielding a 64 bit result. That would be a reasonable result, (and conforms to what many/most hardware multipliers do), but does not conform to C rules. In C, this:<code> i64 sz = (pPager-\>pageSize+4)*pRel-\>iSubRec; </code>, where the pageSize and iSubRec members are both 32-bit objects, means: Multiply these 32-bit integers to produce a 32-bit integer, then promote that integer to 64 bits (by sign bit extension or 0-fill according to signedness of the operands) and transfer into the 64-bit assignee. This is counter-intuitive, but it avoids a language design issue where type must flow toward leaves of expression trees.
So the anon-OP is right, and a fix is necessary? Not quite clear from your post.
My point went only to Gunter's assertion about multiplication details in C, not the OP's bug report. As can be seen in [Richard's fix](https://www.sqlite.org/src/info/f4a552ed9f4ab355), he thinks a fix is necessary. I had gotten only to determining the limits of each operand's value<sup>a</sup> before I saw his determination that an overflow needed to be avoided. ---- a. Whether an overflow was possible or not depends on the actual maximum operand values rather than just their types. So a deeper code analysis was required than what was evident in the OP's statements.
(7) By Gunter Hick (gunter_hick) on 2021-09-14 06:11:22 in reply to 4 [link]
I asked my C compiler (GCC 4.8.5/RH7/IA64) about this: int32_t a = 65536; int32_t b = 0x7ffffff; int64_t p; p = a*b; and got this 32 bit init: c7 45 fc 00 00 01 00 movl $0x10000,-0x4(%rbp) 32 bit init: c7 45 f8 ff ff ff 07 movl $0x7ffffff,-0x8(%rbp) 32 bit load: 8b 45 fc mov -0x4(%rbp),%eax 32 bit mult: 0f af 45 f8 imul -0x8(%rbp),%eax 32->64 conv: 48 98 cltq 64 bit stor: 48 89 45 f0 mov %rax,-0x10(%rbp) and then about this: int32_t a = 65536; int32_t b = 0x7ffffff; int64_t p; p = (int64_t)a*b; 32 bit init: c7 45 fc 00 00 01 00 movl $0x10000,-0x4(%rbp) 32 bit init: c7 45 f8 ff ff ff 07 movl $0x7ffffff,-0x8(%rbp) 32 bit load: 8b 45 fc mov -0x4(%rbp),%eax 64 bit cast: 48 63 d0 movslq %eax,%rdx 32 bit load: 8b 45 f8 mov -0x8(%rbp),%eax 32->64 conv: 48 98 cltq 64 bit mult: 48 0f af c2 imul %rdx,%rax 64 bit stor: 48 89 45 f0 mov %rax,-0x10(%rbp) So yes, there may be truncation without a cast. Machine code is 6 bytes longer and 64 bit multiplication seems to require both operands in registers.