pg.ddx.io pgsql-committers@postgresql.org mailing list archivehelp / color / mirror / Atom feed
pgsql: Avoid integer overflow while testing wal_skip_threshold conditio 6+ messages / 1 participants [nested] [flat]
* pgsql: Avoid integer overflow while testing wal_skip_threshold conditio @ 2025-01-30 20:36 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 6+ messages in thread From: Tom Lane @ 2025-01-30 20:36 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Avoid integer overflow while testing wal_skip_threshold condition. smgrDoPendingSyncs had two distinct risks of integer overflow while deciding which way to ensure durability of a newly-created relation. First, it accumulated the total size of all forks in a variable of type BlockNumber (uint32). While we restrict an individual fork's size to fit in that, I don't believe there's such a restriction on all of them added together. Second, it proceeded to multiply the sum by BLCKSZ, which most certainly could overflow a uint32. (The exact expression is total_blocks * BLCKSZ / 1024. The compiler might choose to optimize that to total_blocks * 8, which is not at quite as much risk of overflow as a literal reading would be, but it's still wrong.) If an overflow did occur it could lead to a poor choice to shove a very large relation into WAL instead of fsync'ing it. This wouldn't be fatal, but it could be inefficient. Change total_blocks to uint64 which should be plenty, and rearrange the comparison calculation to be overflow-safe. I noticed this while looking for ramifications of the proposed change in MAX_KILOBYTES. It's not entirely clear to me why wal_skip_threshold is limited to MAX_KILOBYTES in the first place, but in any case this code is unsafe regardless of the range of wal_skip_threshold. Oversight in c6b92041d which introduced wal_skip_threshold, so back-patch to v13. Discussion: https://postgr.es/m/1a01f0-66ec2d80-3b-68487680@27595217 Backpatch-through: 13 Branch ------ REL_17_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/1e25cdb214543d8b661cf01bbdb6f8e2b1a0381e Modified Files -------------- src/backend/catalog/storage.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) ^ permalink raw reply [nested|flat] 6+ messages in thread
* pgsql: Avoid integer overflow while testing wal_skip_threshold conditio @ 2025-01-30 20:36 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 6+ messages in thread From: Tom Lane @ 2025-01-30 20:36 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Avoid integer overflow while testing wal_skip_threshold condition. smgrDoPendingSyncs had two distinct risks of integer overflow while deciding which way to ensure durability of a newly-created relation. First, it accumulated the total size of all forks in a variable of type BlockNumber (uint32). While we restrict an individual fork's size to fit in that, I don't believe there's such a restriction on all of them added together. Second, it proceeded to multiply the sum by BLCKSZ, which most certainly could overflow a uint32. (The exact expression is total_blocks * BLCKSZ / 1024. The compiler might choose to optimize that to total_blocks * 8, which is not at quite as much risk of overflow as a literal reading would be, but it's still wrong.) If an overflow did occur it could lead to a poor choice to shove a very large relation into WAL instead of fsync'ing it. This wouldn't be fatal, but it could be inefficient. Change total_blocks to uint64 which should be plenty, and rearrange the comparison calculation to be overflow-safe. I noticed this while looking for ramifications of the proposed change in MAX_KILOBYTES. It's not entirely clear to me why wal_skip_threshold is limited to MAX_KILOBYTES in the first place, but in any case this code is unsafe regardless of the range of wal_skip_threshold. Oversight in c6b92041d which introduced wal_skip_threshold, so back-patch to v13. Discussion: https://postgr.es/m/1a01f0-66ec2d80-3b-68487680@27595217 Backpatch-through: 13 Branch ------ REL_16_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/f7a08b6e96eeedde1104a68220a9717ceb2c4ec9 Modified Files -------------- src/backend/catalog/storage.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) ^ permalink raw reply [nested|flat] 6+ messages in thread
* pgsql: Avoid integer overflow while testing wal_skip_threshold conditio @ 2025-01-30 20:36 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 6+ messages in thread From: Tom Lane @ 2025-01-30 20:36 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Avoid integer overflow while testing wal_skip_threshold condition. smgrDoPendingSyncs had two distinct risks of integer overflow while deciding which way to ensure durability of a newly-created relation. First, it accumulated the total size of all forks in a variable of type BlockNumber (uint32). While we restrict an individual fork's size to fit in that, I don't believe there's such a restriction on all of them added together. Second, it proceeded to multiply the sum by BLCKSZ, which most certainly could overflow a uint32. (The exact expression is total_blocks * BLCKSZ / 1024. The compiler might choose to optimize that to total_blocks * 8, which is not at quite as much risk of overflow as a literal reading would be, but it's still wrong.) If an overflow did occur it could lead to a poor choice to shove a very large relation into WAL instead of fsync'ing it. This wouldn't be fatal, but it could be inefficient. Change total_blocks to uint64 which should be plenty, and rearrange the comparison calculation to be overflow-safe. I noticed this while looking for ramifications of the proposed change in MAX_KILOBYTES. It's not entirely clear to me why wal_skip_threshold is limited to MAX_KILOBYTES in the first place, but in any case this code is unsafe regardless of the range of wal_skip_threshold. Oversight in c6b92041d which introduced wal_skip_threshold, so back-patch to v13. Discussion: https://postgr.es/m/1a01f0-66ec2d80-3b-68487680@27595217 Backpatch-through: 13 Branch ------ REL_15_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/b296e55b4272494c54a7a65bec99d60200480051 Modified Files -------------- src/backend/catalog/storage.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) ^ permalink raw reply [nested|flat] 6+ messages in thread
* pgsql: Avoid integer overflow while testing wal_skip_threshold conditio @ 2025-01-30 20:36 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 6+ messages in thread From: Tom Lane @ 2025-01-30 20:36 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Avoid integer overflow while testing wal_skip_threshold condition. smgrDoPendingSyncs had two distinct risks of integer overflow while deciding which way to ensure durability of a newly-created relation. First, it accumulated the total size of all forks in a variable of type BlockNumber (uint32). While we restrict an individual fork's size to fit in that, I don't believe there's such a restriction on all of them added together. Second, it proceeded to multiply the sum by BLCKSZ, which most certainly could overflow a uint32. (The exact expression is total_blocks * BLCKSZ / 1024. The compiler might choose to optimize that to total_blocks * 8, which is not at quite as much risk of overflow as a literal reading would be, but it's still wrong.) If an overflow did occur it could lead to a poor choice to shove a very large relation into WAL instead of fsync'ing it. This wouldn't be fatal, but it could be inefficient. Change total_blocks to uint64 which should be plenty, and rearrange the comparison calculation to be overflow-safe. I noticed this while looking for ramifications of the proposed change in MAX_KILOBYTES. It's not entirely clear to me why wal_skip_threshold is limited to MAX_KILOBYTES in the first place, but in any case this code is unsafe regardless of the range of wal_skip_threshold. Oversight in c6b92041d which introduced wal_skip_threshold, so back-patch to v13. Discussion: https://postgr.es/m/1a01f0-66ec2d80-3b-68487680@27595217 Backpatch-through: 13 Branch ------ REL_14_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/6a33bb35c0411e298c99dfd4b40772c3f91f08ad Modified Files -------------- src/backend/catalog/storage.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) ^ permalink raw reply [nested|flat] 6+ messages in thread
* pgsql: Avoid integer overflow while testing wal_skip_threshold conditio @ 2025-01-30 20:36 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 6+ messages in thread From: Tom Lane @ 2025-01-30 20:36 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Avoid integer overflow while testing wal_skip_threshold condition. smgrDoPendingSyncs had two distinct risks of integer overflow while deciding which way to ensure durability of a newly-created relation. First, it accumulated the total size of all forks in a variable of type BlockNumber (uint32). While we restrict an individual fork's size to fit in that, I don't believe there's such a restriction on all of them added together. Second, it proceeded to multiply the sum by BLCKSZ, which most certainly could overflow a uint32. (The exact expression is total_blocks * BLCKSZ / 1024. The compiler might choose to optimize that to total_blocks * 8, which is not at quite as much risk of overflow as a literal reading would be, but it's still wrong.) If an overflow did occur it could lead to a poor choice to shove a very large relation into WAL instead of fsync'ing it. This wouldn't be fatal, but it could be inefficient. Change total_blocks to uint64 which should be plenty, and rearrange the comparison calculation to be overflow-safe. I noticed this while looking for ramifications of the proposed change in MAX_KILOBYTES. It's not entirely clear to me why wal_skip_threshold is limited to MAX_KILOBYTES in the first place, but in any case this code is unsafe regardless of the range of wal_skip_threshold. Oversight in c6b92041d which introduced wal_skip_threshold, so back-patch to v13. Discussion: https://postgr.es/m/1a01f0-66ec2d80-3b-68487680@27595217 Backpatch-through: 13 Branch ------ REL_13_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/49a38238e2b63b24c0b183bdf35866de546f5fef Modified Files -------------- src/backend/catalog/storage.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) ^ permalink raw reply [nested|flat] 6+ messages in thread
* pgsql: Avoid integer overflow while testing wal_skip_threshold conditio @ 2025-01-30 20:36 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 6+ messages in thread From: Tom Lane @ 2025-01-30 20:36 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Avoid integer overflow while testing wal_skip_threshold condition. smgrDoPendingSyncs had two distinct risks of integer overflow while deciding which way to ensure durability of a newly-created relation. First, it accumulated the total size of all forks in a variable of type BlockNumber (uint32). While we restrict an individual fork's size to fit in that, I don't believe there's such a restriction on all of them added together. Second, it proceeded to multiply the sum by BLCKSZ, which most certainly could overflow a uint32. (The exact expression is total_blocks * BLCKSZ / 1024. The compiler might choose to optimize that to total_blocks * 8, which is not at quite as much risk of overflow as a literal reading would be, but it's still wrong.) If an overflow did occur it could lead to a poor choice to shove a very large relation into WAL instead of fsync'ing it. This wouldn't be fatal, but it could be inefficient. Change total_blocks to uint64 which should be plenty, and rearrange the comparison calculation to be overflow-safe. I noticed this while looking for ramifications of the proposed change in MAX_KILOBYTES. It's not entirely clear to me why wal_skip_threshold is limited to MAX_KILOBYTES in the first place, but in any case this code is unsafe regardless of the range of wal_skip_threshold. Oversight in c6b92041d which introduced wal_skip_threshold, so back-patch to v13. Discussion: https://postgr.es/m/1a01f0-66ec2d80-3b-68487680@27595217 Backpatch-through: 13 Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/b9aa4166fa3823d4f1f76286ca21fcfa991ce036 Modified Files -------------- src/backend/catalog/storage.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) ^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2025-01-30 20:36 UTC | newest] Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2025-01-30 20:36 pgsql: Avoid integer overflow while testing wal_skip_threshold conditio Tom Lane <tgl@sss.pgh.pa.us> 2025-01-30 20:36 pgsql: Avoid integer overflow while testing wal_skip_threshold conditio Tom Lane <tgl@sss.pgh.pa.us> 2025-01-30 20:36 pgsql: Avoid integer overflow while testing wal_skip_threshold conditio Tom Lane <tgl@sss.pgh.pa.us> 2025-01-30 20:36 pgsql: Avoid integer overflow while testing wal_skip_threshold conditio Tom Lane <tgl@sss.pgh.pa.us> 2025-01-30 20:36 pgsql: Avoid integer overflow while testing wal_skip_threshold conditio Tom Lane <tgl@sss.pgh.pa.us> 2025-01-30 20:36 pgsql: Avoid integer overflow while testing wal_skip_threshold conditio Tom Lane <tgl@sss.pgh.pa.us>
This inbox is served by DDX for PostgreSQL; see mirroring instructions for how to clone and mirror all data and code used for this inbox