Fix coverity issue in block::initEmptyBlock

[lplewa]

Description

getObjectSize should not return values bigger then 2^16-1. We are assigning it's return value, which is 32 bit to 16 bit one, so it is good to assert it anyway.

Fixes # - issue number(s) if exists

Type of change

Choose one or multiple, leave empty if none of the other choices apply

Add a respective label(s) to PR if you have permissions

• bug fix - change that fixes an issue
• new feature - change that adds functionality
• tests - change in tests
• infrastructure - change in infrastructure and CI
• documentation - documentation update

Tests

• added - required for new features and some bug fixes
• not needed

Documentation

• updated in # - add PR number
• needs to be updated
• not needed

Breaks backward compatibility

• Yes
• No
• Unknown

Notify the following users

List users with @ to send notifications

Other information

[Alexandr-Konovalov]

If result of getObjectSize() must fit to uint16_t, why not change it return type to uint16_t? And move the related assert inside.

[lplewa]

If result of getObjectSize() must fit to uint16_t, why not change it return type to uint16_t? And move the related assert inside.

Because it is scary change to do few days before deadline. To do it correctly we should change it from unsigned int to unsigned short in few places. TBH i would rewrite this function anyway at it has other issues, but few days before deadline, i just want fix coverity and do not create new issues.

I checked that this code is correct, and added assert just to make coverity also aware that it code is fine.

[Alexandr-Konovalov]

Because it is scary change to do few days before deadline.

To late in the release circle is a good point. Could the TODO be added, to keep the intention explicit?

I'm just curious, is moving the assert to getObjectSize() body can make coverity happy? Because for a human reader it may be unclear, why on another call path a result of getObjectSize() is not covered by assert.

[lplewa]

Because it is scary change to do few days before deadline.

To late in the release circle is a good point. Could the TODO be added, to keep the intention explicit?

I'm just curious, is moving the assert to getObjectSize() body can make coverity happy? Because for a human reader it may be unclear, why on another call path a result of getObjectSize() is not covered by assert.

I added TODO.

IMHO readability depends how you read this code. If thru GH maybe. If you use IDE then you can check that block::objectSize is uint_16_t so it make sense to assert before assessment were you do "type cast".

[Alexandr-Konovalov]

Because it is scary change to do few days before deadline.

To late in the release circle is a good point. Could the TODO be added, to keep the intention explicit?
I'm just curious, is moving the assert to getObjectSize() body can make coverity happy? Because for a human reader it may be unclear, why on another call path a result of getObjectSize() is not covered by assert.

I added TODO.

IMHO readability depends how you read this code. If thru GH maybe. If you use IDE then you can check that block::objectSize is uint_16_t so it make sense to assert before assessment were you do "type cast".

Sorry, I'm not totally understand you position wrt moving the assert inside getObjectSize() body. Is it too late to run Coverity once again, so we don't want to risk not to resolve the Coverity diagnostics?

[lplewa]

Because it is scary change to do few days before deadline.

To late in the release circle is a good point. Could the TODO be added, to keep the intention explicit?
I'm just curious, is moving the assert to getObjectSize() body can make coverity happy? Because for a human reader it may be unclear, why on another call path a result of getObjectSize() is not covered by assert.

I added TODO.
IMHO readability depends how you read this code. If thru GH maybe. If you use IDE then you can check that block::objectSize is uint_16_t so it make sense to assert before assessment were you do "type cast".

Sorry, I'm not totally understand you position wrt moving the assert inside getObjectSize() body. Is it too late to run Coverity once again, so we don't want to risk not to resolve the Coverity diagnostics?

This is one argument.
But main argument is that, in my option, we should assert it just before type conversion. So as we are assigning int to short, we are doing assertion that it is safe.

Also sure if you seen getObjectSize() function but it is literally return getIndexOrObjectSize<false>(size); so assert will have no sense here. we could put it to getIndexOrObjectSize() then this function is a mess, with multiple returns, so there in no good place to put this assert.

[Alexandr-Konovalov]

LGTM.

[KFilipek]

Generally, it breaks nothing, but getIndexOrObjectSize() should get more error handling to resolve the issue: