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

Reduce number of malloc/free call in XMSS/external #1724

Merged
merged 9 commits into from
Mar 30, 2024

Conversation

ducnguyen-sb
Copy link
Contributor

@ducnguyen-sb ducnguyen-sb commented Mar 10, 2024

Reduce number of malloc/free call in XMSS/external

Reason:

Solutions:

  1. Add each individual check each places or
  2. Refactor the low-level functions to use temporary buffer provided by high-level functions.

Approach 1 is the easiest, but create too much overhead, and heap is slow.
Approach 2 require rewrite a bit function logic to allocate memory as buffer from high-level function to low-level function.

I select approach 2.

Should out of memory at crypto implementation exit the program?

This is a bit overkill, but I think it's very time-consuming task to rewrite the primitive crypto implementation.

  1. When Out of Memory happen, the normal check is to make an early exit. By chance, this would make the primitive crypto continue to run and perhaps able to generate a signature.
    e.g: A hash tree requires memory allocation to compute the root, somehow the memory allocation fails, the root is returned with garbage value. Because it's hash, there is no way to distinguish random and garbage stack memory.
    Perhaps we should treat this as black box, when primitive crypto out of memory, EXIT the program.
  2. This approach will make using a crypto primitive a pain in the ass to the application. Abrupt exit is normally unacceptable. But when it's out of memory, would we choose to stop generate crypto or.. continue to generate the incorrect one?

Of course these two problems can be solved if we refactor crypto primitives to return all the way to the top level like Rust language does. With C, I don't know how.

Okay, so that's why you see me use normal malloc and NULL check.
I can always switch to OQS_checked_malloc APIs in the XMSS/external.
Let me know.

Others

Some formats here and there to make astyle happy, and convert TODO lines to TODO (from upstream) because it's not from me.

  • Does this PR change the input/output behaviour of a cryptographic algorithm (i.e., does it change known answer test values)? (If so, a version bump will be required from x.y.z to x.(y+1).0.)
  • Does this PR change the list of algorithms available -- either adding, removing, or renaming? Does this PR otherwise change an API? (If so, PRs in fully supported downstream projects dependent on these, i.e., oqs-provider will also need to be ready for review and merge by the time this is merged.)

@cothan
Copy link
Contributor

cothan commented Mar 22, 2024

@SWilson4 , can you take a look at this PR?

@cothan cothan merged commit 52fe37a into stateful-sigs Mar 30, 2024
85 checks passed
@cothan cothan deleted the reduce_malloc_free_calls branch March 30, 2024 16:53
cothan pushed a commit that referenced this pull request Apr 2, 2024
* remove unused file

* move malloc from prf and prf_keygen to external, reduce number of malloc/free calls

* push malloc/free to top level function

* continue to move malloc/free to upper level

* clean up

* modify TODO to TODO(from upstream)

* make astyle happy

* clean up

* use malloc and NULL check
SWilson4 pushed a commit that referenced this pull request Apr 12, 2024
* remove unused file

* move malloc from prf and prf_keygen to external, reduce number of malloc/free calls

* push malloc/free to top level function

* continue to move malloc/free to upper level

* clean up

* modify TODO to TODO(from upstream)

* make astyle happy

* clean up

* use malloc and NULL check
SWilson4 pushed a commit that referenced this pull request Apr 12, 2024
* remove unused file

* move malloc from prf and prf_keygen to external, reduce number of malloc/free calls

* push malloc/free to top level function

* continue to move malloc/free to upper level

* clean up

* modify TODO to TODO(from upstream)

* make astyle happy

* clean up

* use malloc and NULL check
SWilson4 pushed a commit that referenced this pull request Apr 12, 2024
* remove unused file

* move malloc from prf and prf_keygen to external, reduce number of malloc/free calls

* push malloc/free to top level function

* continue to move malloc/free to upper level

* clean up

* modify TODO to TODO(from upstream)

* make astyle happy

* clean up

* use malloc and NULL check
SWilson4 pushed a commit that referenced this pull request May 14, 2024
* remove unused file

* move malloc from prf and prf_keygen to external, reduce number of malloc/free calls

* push malloc/free to top level function

* continue to move malloc/free to upper level

* clean up

* modify TODO to TODO(from upstream)

* make astyle happy

* clean up

* use malloc and NULL check
cothan added a commit that referenced this pull request May 30, 2024
commit 244288f Add XMSS parameter xmss_sha256_h10 (#1482)
commit a7e26d9 Add 12 XMSS and 16 XMSSMT parameters. (#1489)
commit 4694fc3 Add secret key object to XMSS (#1530)
commit 99067be Add XMSS Serialize/Deserialize  (#1542)
commit 2dbfc40 Update XMSS secret key object APIs, sync with LMS  (#1588)
commit 47740ad Enforce idx from unsigned int to uint32_t. (#1611)
commit 9610576 Fix windows-x86 and arm compiling error. (#1634)
commit bb658b7 Address  stateful-sigs comments in #1650 (#1656)
commit 7db8ddf Update `sig_stfl.h` document for #1650 (#1655)
commit c3e5750 Add Apache 2.0 and MIT License to XMSS (#1662)
commit e1f02b2 Change XMSS License from `(Apache 2.0 AND MIT)` to `(Apache 2.0 OR MIT) AND CC0-1.0` (#1697)
commit 17c12c3 Add return status for XMSS lock/unlock functions. (#1712)
commit 1941636 Add return check for lock/unlock function (#1727)
commit b45415c Use `abort()` instead of exit to get the trace log. (#1728)
commit ba63672 Reduce number of `malloc/free` call in `XMSS/external` (#1724)

Signed-off-by: Duc Tri Nguyen <[email protected]>
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 this pull request may close these issues.

3 participants