Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

find bug: Long integer digit decoding error #49

Closed
gaohtao opened this issue Oct 17, 2023 · 3 comments · Fixed by #51
Closed

find bug: Long integer digit decoding error #49

gaohtao opened this issue Oct 17, 2023 · 3 comments · Fixed by #51

Comments

@gaohtao
Copy link

gaohtao commented Oct 17, 2023

I used the C code generated by cbexigen and found that the encoding of long integer digits was correct, but the decoding process was wrong. When the number of long integers exceeds 15 digits, an integer overflow error occurs during the accumulation process, resulting in incorrect calculation results.
The correct code should be following:
int exi_basetypes_convert_64_from_unsigned(exi_unsigned_t* exi_unsigned, uint64_t* value)
{
if (exi_unsigned->octets_count > EXI_BASETYPES_UINT64_MAX_OCTETS)
{
return EXI_ERROR__OCTET_COUNT_LARGER_THAN_TYPE_SUPPORTS;
}

uint8_t* current_octet = exi_unsigned->octets;
*value = 0;

for (size_t n = 0; n < exi_unsigned->octets_count; n++)
{  //fix bug:  overflow of int,  Forced Typecasting to uint64_t,  Tom.hongtao.gao
    //*value = (uint64_t)(*value + ((*current_octet & EXI_BASETYPES_OCTET_SEQ_VALUE_MASK) << (n * 7)));  
    *value = (uint64_t)(*value + ((uint64_t)(*current_octet & EXI_BASETYPES_OCTET_SEQ_VALUE_MASK) << (n * 7)));
    current_octet++;
}

return EXI_ERROR__NO_ERROR;

}

@barsnick
Copy link
Contributor

Hello @gaohtao,
you are absolutely right. current_octet is only promoted to int automatically, when shifting. It needs to be typecasted directly.

We need to review the remaining code, and create a test case for this.

I guess in ISO 15118-2, TMeter and EVSETimeStamp are affected. What did you notice this with?

@gaohtao
Copy link
Author

gaohtao commented Oct 17, 2023

I tested ISO15118-2 protocol SessionSetupRes command, which contains a timestamp defined as int64, I intentionally filled in the timestamp as largeInteger, and I found that the decoded number is wrong.
"{"V2G_Message": {"Header": {"SessionID": "C08CDF36985A7190"}, "Body": {"SessionSetupRes": {"ResponseCode": "OK_NewSessionEstablished", "EVSEID": "UK123E1234", "EVSETimeStamp": 1234567890123456789}}}}";

barsnick added a commit that referenced this issue Oct 17, 2023
The left-hand side of a left-shift operation only gets promoted up to "int",
if too small to support the number of bits created. This leads to
truncation in case of resulting values with more than 32 significant bits,
which are expected in a 64-bit value decoder.

Move the typecast to the left-shifted variable. Do the same for the 32-bit
case, for consistency.

Fixes #49

Signed-off-by: Moritz Barsnick <moritz.barsnick@chargebyte.com>
barsnick added a commit that referenced this issue Oct 18, 2023
The left-hand side of a left-shift operation only gets promoted up to "int",
if too small to support the number of bits created. This leads to
truncation in case of resulting values with more than 32 significant bits,
which are expected in a 64-bit value decoder.

Move the typecast to the left-shifted variable. Do the same for the 32-bit
case, for consistency.

Fixes #49

Signed-off-by: Moritz Barsnick <moritz.barsnick@chargebyte.com>
@barsnick
Copy link
Contributor

This should be fixed on branch main with commit ae4f37e. Please be so kind to test.

Thanks for your report!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants