Обсуждение: Bufmgr possible overflow
Hi,
IMO I think that commit 31966b1 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,
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
Вложения
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 */
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 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