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 parameter set but unused on psa_cipher_update_ecb #4938

Conversation

gilles-peskine-arm
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm commented Sep 13, 2021

Fix #4935. The only compiler known to complain so far is the development version of Clang (to be released as Clang 14), which oss-fuzz just switched to.

Needs backport: 2.x

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm gilles-peskine-arm added bug needs-review Every commit must be reviewed by at least two team members, needs-backports Backports are missing or are pending review and approval. needs-reviewer This PR needs someone to pick it up for review labels Sep 13, 2021
This parameter was set but not used, which was pointless. Clang 14 detects
this and legitimately complains.

Remove the parameter. This is an internal function, only called once. The
caller already has a sufficient check on the output buffer size which
applies in more cases, so there is no real gain in robustness in adding the
same check inside the internal function.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm gilles-peskine-arm force-pushed the psa_cipher_update_ecp-unused_parameter branch from 803baa4 to 1716f32 Compare September 13, 2021 07:46
@yanesca yanesca self-requested a review September 13, 2021 07:59
@catenacyber
Copy link
Contributor

Thanks Gilles for working on this fix

yanesca
yanesca previously approved these changes Sep 13, 2021
Copy link
Contributor

@yanesca yanesca left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

The fix for the warning looks good to me, but one point in the documentation looks suspect.

* it is stored in \p ctx for future processing.
* \param output The buffer where the output is written.
* \param output_size The size of \p output in bytes.
* It must be at least `floor((p + input_length) / BS)`
Copy link
Contributor

Choose a reason for hiding this comment

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

That doesn't look right; did you mean to multiply by BS at the end?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops. I started to write the size in blocks, but then thought that bytes would be clearer, but then I forgot to multiply.

@mpg mpg added needs-work 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 Sep 13, 2021
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Sep 13, 2021
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@yanesca yanesca left a comment

Choose a reason for hiding this comment

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

LGTM

@yanesca yanesca removed the needs-review Every commit must be reviewed by at least two team members, label Sep 13, 2021
@yanesca yanesca removed the needs-backports Backports are missing or are pending review and approval. label Sep 13, 2021
@yanesca yanesca merged commit cacec72 into Mbed-TLS:development Sep 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

error: parameter 'output_size' set but not used
4 participants