ndsev / zserio

zero sugar, zero fat, zero serialization overhead
https://zserio.org/
BSD 3-Clause "New" or "Revised" License
108 stars 26 forks source link

Unintentional integer overflow in an expression before final cast to the intentional type (nds core) #659

Open bhanuprakasheagala-at-mbrdi opened 1 month ago

bhanuprakasheagala-at-mbrdi commented 1 month ago

Zserio version and language Zserio: 2.1.2 Language: C++

Describe the bug In NDS Core C++ code, There are instances where there is a potentially overflow expression with type unsigned integer which is evaluated using 32-bit arithmetic, and then used in a context that expects an expression of type _uint64t (unsigned 64 bits)

For example, in the file: core\grid\Grid_varint32.cpp

image

where both the functions getNumColumns() and getSize() are the types of 32 bit unsigned integers.

This is an issue as per the Autosar checker for Cpp guidelines as there is a possibility of overflow of the expression before it's widen(with the final cast to _uint64t).

In these kinds of cases, it would be preferred to perform explicit type cast either of the operands in the expression(in this case, any of the functions) to _uint64t which is the expected final type.

Thankyou

mikir commented 4 weeks ago

Thanks a lot for report. You are right.

One possible fix could be done in the schema (to change at least one type of the operand size or numColumns to uint64 type) but I don't think that this is a good solution.

Another solution could be to redesign Zserio core to improve expressions (#86) together with a cast support. However, this will require significant effort which we cannot afford at the moment. I will initiate discussion what we can do in some reasonable time.

For now, I will move it to the backlog.

mikir commented 1 week ago

I forgot to mention another possible fix in the schema. The 32-bit operator size can be cast by adding the following function to the schema:

function varuint64 size()
{
    return size;
}

In this way, the multiplication will be done using 64-bit arithmetic. However, the change in the schema will be necessary.

One question: Is this issue a blocker for you?