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.

Sign up for GitHub

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

Jump to bottom

Fix dead code in mbedtls_mpi_exp_mod() #2317

Merged
merged 3 commits into from
Mar 6, 2019
Merged

Fix dead code in mbedtls_mpi_exp_mod() #2317

merged 3 commits into from
Mar 6, 2019

Conversation

pkolbus
Copy link
Contributor

@pkolbus pkolbus commented Dec 27, 2018 •
edited
Loading

Description

Address a finding from Coverity's DEADCODE checker in bignum.c when the default MBEDTLS_MPI_WINDOW_SIZE is used. This doesn't result in incorrect code, but depending on the compiler there may be a very tiny ROM and performance improvement. It also makes for one less finding that users of Coverity & mbed TLS need to look at.

Resolves #2309

Status

READY

Requires Backporting

NO

Migrations

NO

Additional comments

n/a

Todos

  • Tests
  • Documentation
  • Changelog updated
  • Backported

Steps to test or reproduce

Existing tests cover the default window size. Added test to all.sh for coverage in the case a different size is used.

@pkolbus pkolbus changed the title Fix dead code Fix dead code in mbedtls_mpi_exp_mod() Dec 27, 2018
@RonEld
Copy link
Contributor

RonEld commented Dec 27, 2018

pending CLA

@RonEld RonEld added bug CLA requested component-crypto Crypto primitives and low-level interfaces labels Dec 27, 2018
@k-stachowiak k-stachowiak self-assigned this Jan 18, 2019
k-stachowiak
k-stachowiak previously approved these changes Jan 18, 2019
View reviewed changes
Copy link
Contributor

@k-stachowiak k-stachowiak left a comment

Choose a reason for hiding this comment

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

The change looks good. Let's wait for the second review.

@k-stachowiak
Copy link
Contributor

retest

AndrzejKurek
AndrzejKurek previously approved these changes Jan 21, 2019
View reviewed changes
@simonbutcher
Copy link
Contributor

CI fails only on the BSD timing test, which can be considered a pass.

@Patater - I think this needs one of your team to approve before I can mark it as 'ready for merge'. You also need to decide if you need backporting. Please confirm.

@Patater Patater added the needs-backports Backports are missing or are pending review and approval. label Jan 28, 2019
This was referenced Jan 29, 2019
Merged
Merged
@k-stachowiak
Copy link
Contributor

@Patater I have raised a backport PRs: #2387 and #2388.

@RonEld
Copy link
Contributor

RonEld commented Jan 31, 2019

Both backports have been fully approved, so removing the "needs backports" label

@RonEld RonEld removed the needs-backports Backports are missing or are pending review and approval. label Jan 31, 2019
@Patater Patater added the approved Design and code approved - may be waiting for CI or backports label Jan 31, 2019
@pkolbus
Fix DEADCODE in mbedtls_mpi_exp_mod()
e6bcad3 
In mbedtls_mpi_exp_mod(), the limit check on wsize is never true when
MBEDTLS_MPI_WINDOW_SIZE is at least 6. Wrap in a preprocessor guard
to remove the dead code and resolve a Coverity finding from the
DEADCODE checker.

Change-Id: Ice7739031a9e8249283a04de11150565b613ae89
@pkolbus
all.sh: Test MBEDTLS_MPI_WINDOW_SIZE=1
60c6da2 
There were no tests for a non-default MPI window size. Add one.

Change-Id: Ic08fbc9161d0b3ee67eb3c91f9baf602646c9dfe
@pkolbus pkolbus dismissed stale reviews from AndrzejKurek and k-stachowiak via 60c6da2 February 1, 2019 12:43
@pkolbus
Copy link
Contributor Author

pkolbus commented Feb 1, 2019

Rebased to account for the structural changes in all.sh (#2325). Only the second patch had merge conflicts.

k-stachowiak
k-stachowiak previously approved these changes Feb 1, 2019
View reviewed changes
Copy link
Contributor

@k-stachowiak k-stachowiak left a comment

Choose a reason for hiding this comment

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

@pkolbus Thanks for the rebase! Looks good.

@Patater Patater added the needs-backports Backports are missing or are pending review and approval. label Feb 4, 2019
@hanno-becker
Copy link

@pkolbus Thank you for another contribution! Even though it's a small fix, could you please add an entry to the ChangeLog describing the change and crediting yourself? Suggestion:

Changes
   * Remove dead code from bignum.c in the default configuration.
     Found by Coverity, reported and fixed by YOUR_NAME. Fixes #2309.
   * Add test for minimal value of MBEDTLS_MPI_WINDOW_SIZE to all.sh.
     Contributed by YOUR_NAME.

hanno-becker
hanno-becker suggested changes Feb 8, 2019
View reviewed changes
Copy link

@hanno-becker hanno-becker left a comment

Choose a reason for hiding this comment

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

ChangeLog entry requested. Looks good otherwise, thanks @pkolbus!

@k-stachowiak
Copy link
Contributor

@hanno-arm Thanks for suggesting the change log entries. I have added them in the backport PRs.
@pkolbus You may see the changelog entries in the backports and let me know if you are fine with being credited in such way. If not, please let me know, so that I can fix my entries.

@pkolbus
Copy link
Contributor Author

pkolbus commented Feb 9, 2019

@hanno-arm Thanks for the suggested text. Added.

@k-stachowiak Commented in the backports - only thing is that my employer (Garmin) should get a mention as well.

k-stachowiak
k-stachowiak approved these changes Feb 11, 2019
View reviewed changes
Copy link
Contributor

@k-stachowiak k-stachowiak left a comment

Choose a reason for hiding this comment

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

@pkolbus Thanks for the quick update. I applied appropriate changes to the backports.

@simonbutcher simonbutcher removed the needs-backports Backports are missing or are pending review and approval. label Feb 19, 2019
@simonbutcher
Copy link
Contributor

Both backports, #2387 and #2388 are now promoted to 'ready for merge', so the 'needs: backports' label can be removed.

@Patater Patater merged commit 5da93f8 into Mbed-TLS:development Mar 6, 2019
@pkolbus pkolbus deleted the mpi-deadcode branch March 7, 2019 02:28
@k-stachowiak k-stachowiak mentioned this pull request Jul 3, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AndrzejKurek AndrzejKurek AndrzejKurek approved these changes

@hanno-becker hanno-becker hanno-becker approved these changes

@k-stachowiak k-stachowiak k-stachowiak approved these changes

Assignees

@k-stachowiak k-stachowiak

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@pkolbus @RonEld @k-stachowiak @simonbutcher @hanno-becker @AndrzejKurek @Patater