ibmtss icon indicating copy to clipboard operation
ibmtss copied to clipboard

Allow usage of user defined malloc, realloc and free functions

Open MiSimon opened this issue 3 months ago • 2 comments

This PR adds the following functions:

  • TSS_Free, wrapper for free or similar function
  • TSS_SetMemoryFunctions, allow the usage of platform specific malloc, realloc and free functions. Must be called before any other TSS function. If not called malloc, realloc and free will be used.

Replaced all calls to free with calls to TSS_Free.

MiSimon avatar Mar 14 '24 11:03 MiSimon

Small comment: It would be nice to explain 'why' this is needed in the pull request. I can add that to the documentation.

Question 1: ifdef for Windows and Posix were added to the three calls. Why is this needed, since the code is the same in both cases?

Question 2: I suspect that calling TSS_SetMemoryFunctions() after a malloc and before a free would be unpredictable. Is there a use case for setting it more than once?

If not, perhaps the function should check for 'already set' and return an error.

If there is a use case for setting it more than once, then perhaps 'all three parameters NULL' should be legal, a way to set back to the default.

Another comment: There's no test case. I'd rather not upstream untested code.

kgoldman avatar Mar 29 '24 18:03 kgoldman

I used the library in UEFI applications and there is no standard library (no malloc, realloc, free,...), but there are direct replacements for most of the standard functions (strlen, strcpy, memcpy,...). I solved this by using defines for most of the standard functions and provided function pointers for malloc, realloc and free.

I created this PR because others may also use the library in environments where the standard library is not available.

Answer 1: You are right that does not make sense, I am working on another PR and this was part of it.

Answer 2: You are correct if you change TSS_SetMemoryFunctions calls to free with pointers that were allocated using the previous malloc are undefined behaviour. I don't think that there is a use case for setting it more then once. I will add a check to ensure it can only be set once.

Answer test case: I used the test scripts but they are not ideal for this kind of PR. Is there already a test collection for such "internal" changes?

MiSimon avatar Apr 02 '24 06:04 MiSimon