Skip to content

Commit

Permalink
Check if TSS_SetMemoryFunctions has already set the memory functions.
Browse files Browse the repository at this point in the history
  • Loading branch information
@MiSimon
MiSimon committed Apr 2, 2024
1 parent 3a34cab commit 0d2515d
Showing 1 changed file with 5 additions and 1 deletion.
6 changes: 5 additions & 1 deletion utils/tssproperties.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -584,7 +584,11 @@ TPM_RC TSS_SetMemoryFunctions(TSS_CUST_MALLOC custom_malloc, TSS_CUST_REALLOC cu
TPM_RC rc = 0;

if (custom_malloc == NULL || custom_realloc == NULL || custom_free == NULL) {
rc = TSS_RC_BAD_PROPERTY_VALUE;
rc = TSS_RC_NULL_PARAMETER;
}

if (tssMalloc != NULL || tssRealloc != NULL || tssFree != NULL) {
rc = TSS_RC_PROPERTY_ALREADY_SET;
}

if (rc == 0) {
Expand Down

2 comments on commit 0d2515d

@kgoldman
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at the openssl implementation and it has a few good ideas.

  1. They protect against setting the new functions once a malloc or realloc has been called.
  2. Putting the setter in tssproperties.c feels misplaced, since it's not a property that can be set at compile time, env variable, etc. It also makes the tssMalloc, etc. variables external globals, which means that they are exposed to the application directly. Openssl puts it all in one file and uses static (inaccessible) variables.

They do a few smaller items that may be worthwhile.

  1. They initialize the static variable, so they don't need the "if set do this, else do this" on every call.
  2. They protect against setting the variable to the function to prevent a loop. I.e., test for custom_malloc == TSS_Malloc.

At some point, would it worthwhile to rebase so we don't have layers of patches?

@MiSimon
Copy link
Author

@MiSimon MiSimon commented on 0d2515d Jul 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, they use a much cleaner scheme for this functionality.

Some things that i would change:

  • All functions must be set at once
  • The loop checks should not be necessary since the function signatures are different, i would stick to initialize the static variables to NULL.

I will modify the PR to mimic the openssl behaviour.

I would like to continue with single patches until we reach the point at which the PR is ready to merge.

Please sign in to comment.