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

fix memory leak in mpi_miller_rabin() #2363

Merged
merged 2 commits into from
Sep 3, 2019

Conversation

jenswi-linaro
Copy link
Contributor

Fixes memory leak in mpi_miller_rabin() that occurs when the function has
failed to obtain a usable random 'A' 30 turns in a row.

Signed-off-by: Jens Wiklander jens.wiklander@linaro.org

Notes:

  • Pull requests cannot be accepted until:
  • This is just a template, so feel free to use/remove the unnecessary things

Description

A few sentences describing the overall goals of the pull request's commits.

Status

READY/IN DEVELOPMENT/HOLD

Requires Backporting

When there is a bug fix, it should be backported to all maintained and supported branches.
Changes do not have to be backported if:

  • This PR is a new feature\enhancement
  • This PR contains changes in the API. If this is true, and there is a need for the fix to be backported, the fix should be handled differently in the legacy branch

Yes | NO
Which branch?

Migrations

If there is any API change, what's the incentive and logic for it.

YES | NO

Additional comments

Any additional information that could be of interest

Todos

  • Tests
  • Documentation
  • Changelog updated
  • Backported

Steps to test or reproduce

Outline the steps to test or reproduce the PR here.

Fixes memory leak in mpi_miller_rabin() that occurs when the function has
failed to obtain a usable random 'A' 30 turns in a row.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
@RonEld RonEld added bug CLA valid needs-review Every commit must be reviewed by at least two team members, component-crypto Crypto primitives and low-level interfaces labels Jan 17, 2019
@RonEld
Copy link
Contributor

RonEld commented Jan 17, 2019

@jenswi-linaro Thank you for your contribution!

Would you be willing to add an entry in the ChangeLog, crediting yourself \ Linaro for the contribution?

@RonEld
Copy link
Contributor

RonEld commented Jan 17, 2019

Note: CLA is valid, as part of the Linaro project CLA

Copy link
Contributor

@RonEld RonEld left a comment

Choose a reason for hiding this comment

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

The code change is good.
However, a ChangeLog entry is required.

I understand that you may not have time. We can add the entry on your behlaf, if you are busy.

@RonEld
Copy link
Contributor

RonEld commented Jan 17, 2019

Note: Requires backporting to mbedtls-2.7 and mbedtls-2.16

@RonEld RonEld added the needs-backports Backports are missing or are pending review and approval. label Jan 17, 2019
@jenswi-linaro
Copy link
Contributor Author

Added a change log entry. Please let me know if this is what you had in mind.

ChangeLog Outdated
@@ -11,6 +11,7 @@ Bugfix
previously lead to a stack overflow on constrained targets.
* Add `MBEDTLS_SELF_TEST` for the mbedtls_self_test functions
in the header files, which missed the precompilation check. #971
* Fix memory leak in in mpi_miller_rabin()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Fix memory leak in in mpi_miller_rabin()
* Fix memory leak in in mpi_miller_rabin(). Contributed by Jens Wiklander in #2363.

Unless you prefer the credit to be given to Linaro.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. I added both. :-)

@RonEld
Copy link
Contributor

RonEld commented Jan 17, 2019

Thank you for adding the ChangeLog entry. I added a suggestion for a change, as we credit the contributors in the ChangeLog.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
@RonEld RonEld removed the needs-work label Jan 18, 2019
@RonEld RonEld requested a review from dgreen-arm January 18, 2019 15:44
@RonEld RonEld removed the needs-review Every commit must be reviewed by at least two team members, label Jan 22, 2019
@simonbutcher simonbutcher added the needs-ci Needs to pass CI tests label Jan 24, 2019
@RonEld
Copy link
Contributor

RonEld commented Jan 31, 2019

Backports available at #2398 and #2399

@RonEld RonEld removed the needs-backports Backports are missing or are pending review and approval. label Aug 22, 2019
@RonEld
Copy link
Contributor

RonEld commented Aug 22, 2019

Backports have been fully approved, so removed the "needs backports" label

@Patater Patater added approved Design and code approved - may be waiting for CI or backports and removed needs-ci Needs to pass CI tests labels Aug 22, 2019
@Patater Patater merged commit 035eaea into Mbed-TLS:development Sep 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Design and code approved - may be waiting for CI or backports bug component-crypto Crypto primitives and low-level interfaces
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants