#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

[Daniel Peintner]

Since I have not received any feedback, I assume that the answer was/is satisfactory.

In summary, it is simply the case that arrays (for character strings, bytes, etc.) must be initialized manually. However, this is also standard practice in C.

[Peter Lawrence]

Sorry for the delay in responding.

Thank you for mulling over this particular library usage artifact.

The approach that I wound up using was to omit all the init_ calls and use memset exclusively. As long as link-time optimization is employed, the executable is smaller without the init_ code (without any code changes to the OpenV2G library), and a single memset effectively does a superset of init_ functionality in less code space. The memset is cheap in terms of CPU usage, and it ensures that only the structures that the user formally initializes are encoded by the library.

I'm not saying your declared reasoning is wrong; I'm just elaborating on the approach that I wound up using instead.

Thanks again.