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

mbedtls_mpi_sub_abs: Skip memcpy when redundant (#6701). #6942

Merged
merged 2 commits into from
Feb 1, 2023

Conversation

ucko
Copy link
Contributor

@ucko ucko commented Jan 17, 2023

Description

In some contexts, the output pointer may equal the first input pointer, in which case copying is not only superfluous but results in "Source and destination overlap in memcpy" errors from Valgrind (as I observed in the context of ecp_double_jac) and a diagnostic message from TrustInSoft Analyzer (as Pascal Cuoq reported in the context of other ECP functions called by cert-app with a suitable certificate).

I will be happy to supply a corresponding LTS PR in due course, particularly given that I'm using that branch myself.

(Split from #6930.)

Gatekeeper checklist

Notes for the submitter

Please refer to the contributing guidelines, especially the
checklist for PR contributors.

In some contexts, the output pointer may equal the first input
pointer, in which case copying is not only superfluous but results in
"Source and destination overlap in memcpy" errors from Valgrind (as I
observed in the context of ecp_double_jac) and a diagnostic message
from TrustInSoft Analyzer (as Pascal Cuoq reported in the context of
other ECP functions called by cert-app with a suitable certificate).

Signed-off-by: Aaron M. Ucko <ucko@ncbi.nlm.nih.gov>
@ucko
Copy link
Contributor Author

ucko commented Jan 17, 2023 via email

@tom-cosgrove-arm tom-cosgrove-arm added needs-review Every commit must be reviewed by at least two team members, component-crypto Crypto primitives and low-level interfaces needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review priority-medium Medium priority - this can be reviewed as time permits labels Jan 17, 2023
Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

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

LGTM

@daverodgman daverodgman added approved Design and code approved - may be waiting for CI or backports single-reviewer This PR qualifies for having only one reviewer needs-backports Backports are missing or are pending review and approval. and removed needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Jan 29, 2023
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

@gilles-peskine-arm gilles-peskine-arm added needs-work and removed approved Design and code approved - may be waiting for CI or backports labels Jan 31, 2023
Signed-off-by: Aaron M. Ucko <ucko@ncbi.nlm.nih.gov>
@ucko
Copy link
Contributor Author

ucko commented Jan 31, 2023 via email

@tom-cosgrove-arm
Copy link
Contributor

Thanks for fixing that; can you prepare the backport now? We normally merge a PR and its backport at the same time, so we need both before this can be merged.

@tom-cosgrove-arm tom-cosgrove-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-work labels Jan 31, 2023
Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

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

LGTM; just needs CI (I don't expect any problems now) and the backport

@ucko
Copy link
Contributor Author

ucko commented Jan 31, 2023 via email

@tom-cosgrove-arm
Copy link
Contributor

tom-cosgrove-arm commented Jan 31, 2023

The backport should be raised as a second PR, targeting the branch mbedtls-2.28, with the same title as the original but with [Backport 2.28] prepended to the title. See, for example, PRs #6982 and #6983. Where possible, we create it using git cherry-pick onto the mbedtls-2.28 branch

@ucko
Copy link
Contributor Author

ucko commented Jan 31, 2023 via email

Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM

@gilles-peskine-arm gilles-peskine-arm merged commit a193986 into Mbed-TLS:development Feb 1, 2023
gouriano pushed a commit to ncbi/ncbi-cxx-toolkit-public that referenced this pull request Jul 12, 2023
It still lacks Mbed-TLS/mbedtls#6942 but now
formally calls __GI_memcpy rather than memcpy@@GLIBC_2.14.
JIRA: CXX-13141.

git-svn-id: https://anonsvn.ncbi.nlm.nih.gov/repos/v1/trunk/c++@100239 78c7ea69-d796-4a43-9a09-de51944f1b03
gouriano pushed a commit to ncbi/ncbi-cxx-toolkit-public that referenced this pull request Jul 27, 2023
It still lacks Mbed-TLS/mbedtls#6942 but now
formally calls __GI_memcpy rather than memcpy@@GLIBC_2.14.
JIRA: CXX-13141.

git-svn-id: https://anonsvn.ncbi.nlm.nih.gov/repos/v1/trunk/c++@100239 78c7ea69-d796-4a43-9a09-de51944f1b03
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 component-crypto Crypto primitives and low-level interfaces needs-backports Backports are missing or are pending review and approval. needs-ci Needs to pass CI tests priority-medium Medium priority - this can be reviewed as time permits single-reviewer This PR qualifies for having only one reviewer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants