Bufmgr possible overflow
Hi,
IMO I think that commit 31966b1
<https://github.com/postgres/postgres/commit/31966b151e6ab7a6284deab6e8fe5faddaf2ae4c>
has an oversight.
All the logic of the changes are based on the "extend_by" variable, which
is a uint32, but in some places it is using "int", which can lead to an
overflow at some point.
I also take the opportunity to correct another oversight, regarding the
commit dad50f6
<https://github.com/postgres/postgres/commit/dad50f677c42de207168a3f08982ba23c9fc6720>
,
for possible duplicate assignment.
GetLocalBufferDescriptor was called twice.
Taking advantage of this, I promoted a scope reduction for some variables,
which I thought was opportune.
Patch attached.
regards,
Ranier Vilela
Attachments:
001-fix-bufmgr-extend-variable-index.patchapplication/octet-stream; name=001-fix-bufmgr-extend-variable-index.patchDownload
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 7778dde3e5..b7dbe386c6 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -1933,7 +1933,7 @@ ExtendBufferedRelShared(ExtendBufferedWhat eb,
* This needs to happen before we extend the relation, because as soon as
* we do, other backends can start to read in those pages.
*/
- for (int i = 0; i < extend_by; i++)
+ for (uint32 i = 0; i < extend_by; i++)
{
Buffer victim_buf = buffers[i];
BufferDesc *victim_buf_hdr = GetBufferDescriptor(victim_buf - 1);
@@ -2065,7 +2065,7 @@ ExtendBufferedRelShared(ExtendBufferedWhat eb,
io_start, extend_by);
/* Set BM_VALID, terminate IO, and wake up any waiters */
- for (int i = 0; i < extend_by; i++)
+ for (uint32 i = 0; i < extend_by; i++)
{
Buffer buf = buffers[i];
BufferDesc *buf_hdr = GetBufferDescriptor(buf - 1);
diff --git a/src/backend/storage/buffer/localbuf.c b/src/backend/storage/buffer/localbuf.c
index f684862d98..576ae54646 100644
--- a/src/backend/storage/buffer/localbuf.c
+++ b/src/backend/storage/buffer/localbuf.c
@@ -118,9 +118,8 @@ LocalBufferAlloc(SMgrRelation smgr, ForkNumber forkNum, BlockNumber blockNum,
bool *foundPtr)
{
BufferTag newTag; /* identity of requested block */
- LocalBufferLookupEnt *hresult;
BufferDesc *bufHdr;
- Buffer victim_buffer;
+ LocalBufferLookupEnt *hresult;
int bufid;
bool found;
@@ -145,6 +144,7 @@ LocalBufferAlloc(SMgrRelation smgr, ForkNumber forkNum, BlockNumber blockNum,
else
{
uint32 buf_state;
+ Buffer victim_buffer;
victim_buffer = GetLocalVictimBuffer();
bufid = -victim_buffer - 1;
@@ -360,7 +360,7 @@ ExtendBufferedRelLocal(ExtendBufferedWhat eb,
relpath(eb.smgr->smgr_rlocator, fork),
MaxBlockNumber)));
- for (int i = 0; i < extend_by; i++)
+ for (uint32 i = 0; i < extend_by; i++)
{
int victim_buf_id;
BufferDesc *victim_buf_hdr;
@@ -377,7 +377,7 @@ ExtendBufferedRelLocal(ExtendBufferedWhat eb,
hash_search(LocalBufHash, (void *) &tag, HASH_ENTER, &found);
if (found)
{
- BufferDesc *existing_hdr = GetLocalBufferDescriptor(hresult->id);
+ BufferDesc *existing_hdr;
uint32 buf_state;
UnpinLocalBuffer(BufferDescriptorGetBuffer(victim_buf_hdr));
@@ -416,7 +416,7 @@ ExtendBufferedRelLocal(ExtendBufferedWhat eb,
pgstat_count_io_op_time(IOOBJECT_TEMP_RELATION, IOCONTEXT_NORMAL, IOOP_EXTEND,
io_start, extend_by);
- for (int i = 0; i < extend_by; i++)
+ for (uint32 i = 0; i < extend_by; i++)
{
Buffer buf = buffers[i];
BufferDesc *buf_hdr;
@@ -489,7 +489,6 @@ DropRelationLocalBuffers(RelFileLocator rlocator, ForkNumber forkNum,
for (i = 0; i < NLocBuffer; i++)
{
BufferDesc *bufHdr = GetLocalBufferDescriptor(i);
- LocalBufferLookupEnt *hresult;
uint32 buf_state;
buf_state = pg_atomic_read_u32(&bufHdr->state);
@@ -499,6 +498,8 @@ DropRelationLocalBuffers(RelFileLocator rlocator, ForkNumber forkNum,
BufTagGetForkNum(&bufHdr->tag) == forkNum &&
bufHdr->tag.blockNum >= firstDelBlock)
{
+ LocalBufferLookupEnt *hresult;
+
if (LocalRefCount[i] != 0)
elog(ERROR, "block %u of %s is still referenced (local %u)",
bufHdr->tag.blockNum,
@@ -536,7 +537,6 @@ DropRelationAllLocalBuffers(RelFileLocator rlocator)
for (i = 0; i < NLocBuffer; i++)
{
BufferDesc *bufHdr = GetLocalBufferDescriptor(i);
- LocalBufferLookupEnt *hresult;
uint32 buf_state;
buf_state = pg_atomic_read_u32(&bufHdr->state);
@@ -544,6 +544,8 @@ DropRelationAllLocalBuffers(RelFileLocator rlocator)
if ((buf_state & BM_TAG_VALID) &&
BufTagMatchesRelFileLocator(&bufHdr->tag, &rlocator))
{
+ LocalBufferLookupEnt *hresult;
+
if (LocalRefCount[i] != 0)
elog(ERROR, "block %u of %s is still referenced (local %u)",
bufHdr->tag.blockNum,
diff --git a/src/backend/storage/smgr/smgr.c b/src/backend/storage/smgr/smgr.c
index 70d0d570b1..b7b1fac40a 100644
--- a/src/backend/storage/smgr/smgr.c
+++ b/src/backend/storage/smgr/smgr.c
@@ -52,7 +52,7 @@ typedef struct f_smgr
void (*smgr_extend) (SMgrRelation reln, ForkNumber forknum,
BlockNumber blocknum, const void *buffer, bool skipFsync);
void (*smgr_zeroextend) (SMgrRelation reln, ForkNumber forknum,
- BlockNumber blocknum, int nblocks, bool skipFsync);
+ BlockNumber blocknum, uint32 nblocks, bool skipFsync);
bool (*smgr_prefetch) (SMgrRelation reln, ForkNumber forknum,
BlockNumber blocknum);
void (*smgr_read) (SMgrRelation reln, ForkNumber forknum,
@@ -520,7 +520,7 @@ smgrextend(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum,
*/
void
smgrzeroextend(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum,
- int nblocks, bool skipFsync)
+ uint32 nblocks, bool skipFsync)
{
smgrsw[reln->smgr_which].smgr_zeroextend(reln, forknum, blocknum,
nblocks, skipFsync);
diff --git a/src/include/storage/smgr.h b/src/include/storage/smgr.h
index 17fba6f91a..b12951a142 100644
--- a/src/include/storage/smgr.h
+++ b/src/include/storage/smgr.h
@@ -94,7 +94,7 @@ extern void smgrdounlinkall(SMgrRelation *rels, int nrels, bool isRedo);
extern void smgrextend(SMgrRelation reln, ForkNumber forknum,
BlockNumber blocknum, const void *buffer, bool skipFsync);
extern void smgrzeroextend(SMgrRelation reln, ForkNumber forknum,
- BlockNumber blocknum, int nblocks, bool skipFsync);
+ BlockNumber blocknum, uint32 nblocks, bool skipFsync);
extern bool smgrprefetch(SMgrRelation reln, ForkNumber forknum,
BlockNumber blocknum);
extern void smgrread(SMgrRelation reln, ForkNumber forknum,
Perhaps it's a good idea to seprate the patch for each issue.
At Wed, 12 Apr 2023 09:36:14 -0300, Ranier Vilela <ranier.vf@gmail.com> wrote in> IMO I think that commit 31966b1
<https://github.com/postgres/postgres/commit/31966b151e6ab7a6284deab6e8fe5faddaf2ae4c>
has an oversight.All the logic of the changes are based on the "extend_by" variable, which
is a uint32, but in some places it is using "int", which can lead to an
overflow at some point.
int is nowadays is at least 32 bits, so using int in a loop that
iterates up to a uint32 value won't cause an overflow. However, the
fix iteself looks good because it unifies the loop variable types in
similar loops.
On the other hand, I'm not a fan of changing the signature of
smgr_zeroextend to use uint32. I don't think it improves things and
the other reason is that I don't like using unnatural integer types
unnecessarily in API parameter types. ASnyway, the patch causes a type
inconsistency between smgr_zserextend and mdzeroextend.
I also take the opportunity to correct another oversight, regarding the
commit dad50f6
<https://github.com/postgres/postgres/commit/dad50f677c42de207168a3f08982ba23c9fc6720>
,
for possible duplicate assignment.
GetLocalBufferDescriptor was called twice.Taking advantage of this, I promoted a scope reduction for some variables,
which I thought was opportune.
I like the scope reductions.
Regarding the duplicate assignment to existing_hdr, I prefer assigning
it in the definition line, but I don't have a strong opinion on this
matter.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Em qua., 12 de abr. de 2023 às 22:29, Kyotaro Horiguchi <
horikyota.ntt@gmail.com> escreveu:
Perhaps it's a good idea to seprate the patch for each issue.
Thanks Kyotaro for taking a look.
At Wed, 12 Apr 2023 09:36:14 -0300, Ranier Vilela <ranier.vf@gmail.com>
wrote in> IMO I think that commit 31966b1<
https://github.com/postgres/postgres/commit/31966b151e6ab7a6284deab6e8fe5faddaf2ae4c
has an oversight.
All the logic of the changes are based on the "extend_by" variable, which
is a uint32, but in some places it is using "int", which can lead to an
overflow at some point.int is nowadays is at least 32 bits, so using int in a loop that
iterates up to a uint32 value won't cause an overflow.
It's never good to mix data types.
int is signed integer type and can carry only half of the positive numbers
that "unsigned int" can.
from c.h:
#ifndef HAVE_UINT8
typedef unsigned char uint8; /* == 8 bits */
typedef unsigned short uint16; /* == 16 bits */
typedef unsigned int uint32; /* == 32 bits */
#endif /* not HAVE_UINT8 */
However, the
fix iteself looks good because it unifies the loop variable types in
similar loops.
Yeah.
On the other hand, I'm not a fan of changing the signature of
smgr_zeroextend to use uint32. I don't think it improves things and
the other reason is that I don't like using unnatural integer types
unnecessarily in API parameter types.
But ExtendBufferedRelBy calls smgr_zeroextend and carries a uint32 value to
int param.
smgr_zeroextend signature must be changed to work with any values from
uint32.
ASnyway, the patch causes a type
inconsistency between smgr_zserextend and mdzeroextend.
Yeah, have more inconsistency.
extern void smgrwriteback(SMgrRelation reln, ForkNumber forknum,
BlockNumber blocknum, BlockNumber nblocks);
BlockNumber is what integer data type?
I also take the opportunity to correct another oversight, regarding the
commit dad50f6
<https://github.com/postgres/postgres/commit/dad50f677c42de207168a3f08982ba23c9fc6720
,
for possible duplicate assignment.
GetLocalBufferDescriptor was called twice.Taking advantage of this, I promoted a scope reduction for some
variables,
which I thought was opportune.
I like the scope reductions.
Yeah.
Regarding the duplicate assignment to existing_hdr, I prefer assigning
it in the definition line, but I don't have a strong opinion on this
matter.
Closer to where the variable is used is preferable if the assignment is not
cheap.
regards,
Ranier Vilela