#29 corruption unless complete zero-ize of struct iso1EXIDocument output

[Anonymous]

I’ve attached some demonstration code to illustrate. The code decodes a couple of reference ISO1 messages, encodes a response, and compares that response to another reference.

My read of OpenV2G usage is that there are assorted helper functions named with the prefix init_ that are supposed to selectively initialize the right necessary variables to zero. However, I’m finding that only a memset zero-ization of struct iso1EXIDocument prior to building the response produces the desired correct output.

To demonstrate, first compile and run the code as-is to confirm correct operation (no “ERROR” output). Then comment out the “memset(&exiOut, 0, sizeof(exiOut));”, re-compile and run again. Depending on the compiler and build environment, expect to get a “isoenc x” with a negative number (encode_iso1ExiDocument returned an error) or an “ERROR:” when the output is compared against the reference values.

Use of a union to share multiple unrelated structs is the right and proper thing for OpenV2G to do. A possible side-effect is that previous usage of a different union member may leave data uninitialized.

The workaround is to use the memset, which is probably the safe thing to do regardless. However, documenting this and providing example code seemed the right thing to do.

[Daniel Peintner]

Hi,

Thank you for reporting the problem.
For now I did not have more time than running your demo.

I can't really understand why the init_ methods don't work as expected for union.
I'll have to take a closer look at the code.

for example
https://sourceforge.net/p/openv2g/code/HEAD/tree/trunk/src/iso1/iso1EXIDatatypes.c#l44
sets the XXX_isUsed to zero which should prevent to run into other union memory parts.

🤔

Mhhh, I hope I can find time to look into soon... unless you beat me with the explanation.

-- Daniel

[Daniel Peintner]

Hi,

I checked your code and I think there is one missing piece.

OpenV2G does not (properly?) initialize datastructures for bytes and string (internal struct in struct) and expects the user to set the data.

Hence according to my testing
memset(&exiOut, 0, sizeof(exiOut));
is not needed. Remving it does not show any problem with your code.

IF you initialize byte and string arrays.. like you do in

exiOut.V2G_Message.Header.SessionID.bytesLen = 8;
memset(exiOut.V2G_Message.Header.SessionID.bytes, 0xA5, exiOut.V2G_Message.Header.SessionID.bytesLen);

you are fine.

At the moment the init_iso1MessageHeaderType() call looks like this

void init_iso1MessageHeaderType(struct iso1MessageHeaderType* iso1MessageHeaderType) {
iso1MessageHeaderType->Notification_isUsed = 0u;
iso1MessageHeaderType->Signature_isUsed = 0u;
}

while it shoud/could also initialize
iso1MessageHeaderType->SessionID.bytes = ...

Do you agree?

Honestly since it is the user that needs to set data anyway I don't know whether OpenV2G should do it?

What do you think?

-- Daniel