Fix integer overflow in nodeWindowAgg.c

In nodeWindowAgg.c, the calculations for frame start and end positions
in ROWS and GROUPS modes were performed using simple integer addition.
If a user-supplied offset was sufficiently large (close to INT64_MAX),
adding it to the current row or group index could cause a signed
integer overflow, wrapping the result to a negative number.

This led to incorrect behavior where frame boundaries that should have
extended indefinitely (or beyond the partition end) were treated as
falling at the first row, or where valid rows were incorrectly marked
as out-of-frame.  Depending on the specific query and data, these
overflows can result in incorrect query results, execution errors, or
assertion failures.

To fix, use overflow-aware integer addition (ie, pg_add_s64_overflow)
to check for overflows during these additions.  If an overflow is
detected, the boundary is now clamped to INT64_MAX.  This ensures the
logic correctly treats the boundary as extending to the end of the
partition.

Bug: #19405
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Author: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: Tender Wang <tndrwang@gmail.com>
Discussion: https://postgr.es/m/19405-1ecf025dda171555@postgresql.org
Backpatch-through: 14

diff --git a/src/backend/executor/nodeWindowAgg.c b/src/backend/executor/nodeWindowAgg.c
index 9a1acce2b5d366ae8280c8636c106f5172bfe6c2..ed99059afd6508668dfb066a0471f3a5b8d311b7 100644 (file)
--- a/src/backend/executor/nodeWindowAgg.c
+++ b/src/backend/executor/nodeWindowAgg.c
@@ -37,6 +37,7 @@
 #include "catalog/objectaccess.h"
 #include "catalog/pg_aggregate.h"
 #include "catalog/pg_proc.h"
+#include "common/int.h"
 #include "executor/executor.h"
 #include "executor/nodeWindowAgg.h"
 #include "miscadmin.h"
@@ -1465,12 +1466,21 @@ row_is_in_frame(WindowAggState *winstate, int64 pos, TupleTableSlot *slot)
                if (frameOptions & FRAMEOPTION_ROWS)
                {
                        int64           offset = DatumGetInt64(winstate->endOffsetValue);
+                       int64           frameendpos = 0;
 
                        /* rows after current row + offset are out of frame */
                        if (frameOptions & FRAMEOPTION_END_OFFSET_PRECEDING)
                                offset = -offset;
 
-                       if (pos > winstate->currentpos + offset)
+                       /*
+                        * If we have an overflow, it means the frame end is beyond the
+                        * range of int64.  Since currentpos >= 0, this can only be a
+                        * positive overflow.  We treat this as meaning that the frame
+                        * extends to end of partition.
+                        */
+                       if (!pg_add_s64_overflow(winstate->currentpos, offset,
+                                                                        &frameendpos) &&
+                               pos > frameendpos)
                                return -1;
                }
                else if (frameOptions & (FRAMEOPTION_RANGE | FRAMEOPTION_GROUPS))
@@ -1605,7 +1615,16 @@ update_frameheadpos(WindowAggState *winstate)
                        if (frameOptions & FRAMEOPTION_START_OFFSET_PRECEDING)
                                offset = -offset;
 
-                       winstate->frameheadpos = winstate->currentpos + offset;
+                       /*
+                        * If we have an overflow, it means the frame head is beyond the
+                        * range of int64.  Since currentpos >= 0, this can only be a
+                        * positive overflow.  We treat this as being beyond end of
+                        * partition.
+                        */
+                       if (pg_add_s64_overflow(winstate->currentpos, offset,
+                                                                       &winstate->frameheadpos))
+                               winstate->frameheadpos = PG_INT64_MAX;
+
                        /* frame head can't go before first row */
                        if (winstate->frameheadpos < 0)
                                winstate->frameheadpos = 0;
@@ -1717,12 +1736,21 @@ update_frameheadpos(WindowAggState *winstate)
                         * framehead_slot empty.
                         */
                        int64           offset = DatumGetInt64(winstate->startOffsetValue);
-                       int64           minheadgroup;
+                       int64           minheadgroup = 0;
 
                        if (frameOptions & FRAMEOPTION_START_OFFSET_PRECEDING)
                                minheadgroup = winstate->currentgroup - offset;
                        else
-                               minheadgroup = winstate->currentgroup + offset;
+                       {
+                               /*
+                                * If we have an overflow, it means the target group is beyond
+                                * the range of int64.  We treat this as "infinity", which
+                                * ensures the loop below advances to end of partition.
+                                */
+                               if (pg_add_s64_overflow(winstate->currentgroup, offset,
+                                                                               &minheadgroup))
+                                       minheadgroup = PG_INT64_MAX;
+                       }
 
                        tuplestore_select_read_pointer(winstate->buffer,
                                                                                   winstate->framehead_ptr);
@@ -1859,7 +1887,18 @@ update_frametailpos(WindowAggState *winstate)
                        if (frameOptions & FRAMEOPTION_END_OFFSET_PRECEDING)
                                offset = -offset;
 
-                       winstate->frametailpos = winstate->currentpos + offset + 1;
+                       /*
+                        * If we have an overflow, it means the frame tail is beyond the
+                        * range of int64.  Since currentpos >= 0, this can only be a
+                        * positive overflow.  We treat this as being beyond end of
+                        * partition.
+                        */
+                       if (pg_add_s64_overflow(winstate->currentpos, offset,
+                                                                       &winstate->frametailpos) ||
+                               pg_add_s64_overflow(winstate->frametailpos, 1,
+                                                                       &winstate->frametailpos))
+                               winstate->frametailpos = PG_INT64_MAX;
+
                        /* smallest allowable value of frametailpos is 0 */
                        if (winstate->frametailpos < 0)
                                winstate->frametailpos = 0;
@@ -1971,12 +2010,21 @@ update_frametailpos(WindowAggState *winstate)
                         * leave frametailpos = end+1 and frametail_slot empty.
                         */
                        int64           offset = DatumGetInt64(winstate->endOffsetValue);
-                       int64           maxtailgroup;
+                       int64           maxtailgroup = 0;
 
                        if (frameOptions & FRAMEOPTION_END_OFFSET_PRECEDING)
                                maxtailgroup = winstate->currentgroup - offset;
                        else
-                               maxtailgroup = winstate->currentgroup + offset;
+                       {
+                               /*
+                                * If we have an overflow, it means the target group is beyond
+                                * the range of int64.  We treat this as "infinity", which
+                                * ensures the loop below advances to end of partition.
+                                */
+                               if (pg_add_s64_overflow(winstate->currentgroup, offset,
+                                                                               &maxtailgroup))
+                                       maxtailgroup = PG_INT64_MAX;
+                       }
 
                        tuplestore_select_read_pointer(winstate->buffer,
                                                                                   winstate->frametail_ptr);

diff --git a/src/test/regress/expected/window.out b/src/test/regress/expected/window.out
index b86b668f433d0dd9a1a8e707f9fd0c7213bea51d..6ee01f37009626d7f66124a662e92d21bc1c8986 100644 (file)
--- a/src/test/regress/expected/window.out
+++ b/src/test/regress/expected/window.out
@@ -1361,6 +1361,97 @@ SELECT pg_get_viewdef('v_window');
     FROM generate_series(now(), (now() + '@ 100 days'::interval), '@ 1 hour'::interval) i(i);
 (1 row)
 
+-- test overflow frame specifications
+SELECT sum(unique1) over (rows between current row and 9223372036854775807 following exclude current row),
+       unique1, four
+FROM tenk1 WHERE unique1 < 10;
+ sum | unique1 | four 
+-----+---------+------
+  41 |       4 |    0
+  39 |       2 |    2
+  38 |       1 |    1
+  32 |       6 |    2
+  23 |       9 |    1
+  15 |       8 |    0
+  10 |       5 |    1
+   7 |       3 |    3
+   0 |       7 |    3
+     |       0 |    0
+(10 rows)
+
+SELECT sum(unique1) over (rows between 9223372036854775807 following and 1 following),
+       unique1, four
+FROM tenk1 WHERE unique1 < 10;
+ sum | unique1 | four 
+-----+---------+------
+     |       4 |    0
+     |       2 |    2
+     |       1 |    1
+     |       6 |    2
+     |       9 |    1
+     |       8 |    0
+     |       5 |    1
+     |       3 |    3
+     |       7 |    3
+     |       0 |    0
+(10 rows)
+
+SELECT last_value(unique1) over (ORDER BY four rows between current row and 9223372036854775807 following exclude current row),
+       unique1, four
+FROM tenk1 WHERE unique1 < 10;
+ last_value | unique1 | four 
+------------+---------+------
+          7 |       0 |    0
+          7 |       8 |    0
+          7 |       4 |    0
+          7 |       5 |    1
+          7 |       9 |    1
+          7 |       1 |    1
+          7 |       6 |    2
+          7 |       2 |    2
+          7 |       3 |    3
+            |       7 |    3
+(10 rows)
+
+-- These test GROUPS mode with an offset large enough to cause overflow when
+-- added to currentgroup.  Although the overflow doesn't produce visibly wrong
+-- results (due to the incremental nature of group pointer advancement), we
+-- still need to protect against it as signed integer overflow is undefined
+-- behavior in C.
+SELECT sum(unique1) over (ORDER BY four groups between current row and 9223372036854775807 following),
+       unique1, four
+FROM tenk1 WHERE unique1 < 10;
+ sum | unique1 | four 
+-----+---------+------
+  45 |       0 |    0
+  45 |       8 |    0
+  45 |       4 |    0
+  33 |       5 |    1
+  33 |       9 |    1
+  33 |       1 |    1
+  18 |       6 |    2
+  18 |       2 |    2
+  10 |       3 |    3
+  10 |       7 |    3
+(10 rows)
+
+SELECT sum(unique1) over (ORDER BY four groups between 9223372036854775807 following and unbounded following),
+       unique1, four
+FROM tenk1 WHERE unique1 < 10;
+ sum | unique1 | four 
+-----+---------+------
+     |       0 |    0
+     |       8 |    0
+     |       4 |    0
+     |       5 |    1
+     |       9 |    1
+     |       1 |    1
+     |       6 |    2
+     |       2 |    2
+     |       3 |    3
+     |       7 |    3
+(10 rows)
+
 -- RANGE offset PRECEDING/FOLLOWING tests
 SELECT sum(unique1) over (order by four range between 2::int8 preceding and 1::int2 preceding),
        unique1, four

diff --git a/src/test/regress/sql/window.sql b/src/test/regress/sql/window.sql
index 02f105f070e8b419b9884b74d853139a6ffe43db..ff58f45ce2662d34e287a574b829ed676461a6ca 100644 (file)
--- a/src/test/regress/sql/window.sql
+++ b/src/test/regress/sql/window.sql
@@ -330,6 +330,32 @@ CREATE TEMP VIEW v_window AS
 
 SELECT pg_get_viewdef('v_window');
 
+-- test overflow frame specifications
+SELECT sum(unique1) over (rows between current row and 9223372036854775807 following exclude current row),
+       unique1, four
+FROM tenk1 WHERE unique1 < 10;
+
+SELECT sum(unique1) over (rows between 9223372036854775807 following and 1 following),
+       unique1, four
+FROM tenk1 WHERE unique1 < 10;
+
+SELECT last_value(unique1) over (ORDER BY four rows between current row and 9223372036854775807 following exclude current row),
+       unique1, four
+FROM tenk1 WHERE unique1 < 10;
+
+-- These test GROUPS mode with an offset large enough to cause overflow when
+-- added to currentgroup.  Although the overflow doesn't produce visibly wrong
+-- results (due to the incremental nature of group pointer advancement), we
+-- still need to protect against it as signed integer overflow is undefined
+-- behavior in C.
+SELECT sum(unique1) over (ORDER BY four groups between current row and 9223372036854775807 following),
+       unique1, four
+FROM tenk1 WHERE unique1 < 10;
+
+SELECT sum(unique1) over (ORDER BY four groups between 9223372036854775807 following and unbounded following),
+       unique1, four
+FROM tenk1 WHERE unique1 < 10;
+
 -- RANGE offset PRECEDING/FOLLOWING tests
 
 SELECT sum(unique1) over (order by four range between 2::int8 preceding and 1::int2 preceding),