[REPORT] Static analys warnings

Started by Ranier Vilelaover 6 years ago3 messageshackers
Beta feature

Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.

never appliedCI history
Jump to latest
#1Ranier Vilela
ranier.vf@gmail.com

1. Warning: the right operand to | always evaluates to 0

src/include/storage/bufpage.h
#define PAI_OVERWRITE (1 << 0)
#define PAI_IS_HEAP (1 << 1)

#define PageAddItem(page, item, size, offsetNumber, overwrite, is_heap) \
PageAddItemExtended(page, item, size, offsetNumber, \
((overwrite) ? PAI_OVERWRITE : 0) | \
((is_heap) ? PAI_IS_HEAP : 0))

Typo | should be ||:
((overwrite) ? PAI_OVERWRITE : 0) || \
((is_heap) ? PAI_IS_HEAP : 0))

regards,
Ranier Vilela

#2Andres Freund
andres@anarazel.de
In reply to: Ranier Vilela (#1)
Re: [REPORT] Static analys warnings

On 2020-05-03 17:05:54 -0300, Ranier Vilela wrote:

1. Warning: the right operand to | always evaluates to 0

src/include/storage/bufpage.h
#define PAI_OVERWRITE (1 << 0)
#define PAI_IS_HEAP (1 << 1)

#define PageAddItem(page, item, size, offsetNumber, overwrite, is_heap) \
PageAddItemExtended(page, item, size, offsetNumber, \
((overwrite) ? PAI_OVERWRITE : 0) | \
((is_heap) ? PAI_IS_HEAP : 0))

Typo | should be ||:
((overwrite) ? PAI_OVERWRITE : 0) || \
((is_heap) ? PAI_IS_HEAP : 0))

Definitely not. PageAddItemExtended's flags argument is not a boolean,
it's a flag bitmask. It'd entirely break the semantics to make the
change you suggest.

Nor do I know what this warning is about, because clearly in the general
case the right argument to the | does not generally evaluate to 0. I
guess this about a particular use of the macro (with a constant
argument), rather than the macro itself.

#3Ranier Vilela
ranier.vf@gmail.com
In reply to: Andres Freund (#2)
Re: [REPORT] Static analys warnings

Fix possible overflow when converting, possible negative number to uint16.

postingoff can be -1,when converts to uint16, overflow can raise.
Otherwise, truncation can be occurs, losing precision, from int (31 bits)
to uint16 (15 bits)
There is a little confusion in the parameters of some functions in this
file, postigoff is declared as int, other declared as uint16.

src/backend/access/nbtree/nbtinsert.c
static void _bt_insertonpg(Relation rel, BTScanInsert itup_key,
Buffer buf,
Buffer cbuf,
BTStack stack,
IndexTuple itup,
Size itemsz,
OffsetNumber newitemoff,
int postingoff, // INT
bool split_only_page);
static Buffer _bt_split(Relation rel, BTScanInsert itup_key, Buffer buf,
Buffer cbuf, OffsetNumber newitemoff, Size newitemsz,
IndexTuple newitem, IndexTuple orignewitem,
IndexTuple nposting, uint16 postingoff); // UINT16

regards,
Ranier Vilela

Attachments:

fix_possible_overflow_postingoff.patchapplication/octet-stream; name=fix_possible_overflow_postingoff.patchDownload+3-2