Skip to content

Add parentheses to unsafe macros#716

Open
bjwtaylor wants to merge 3 commits into
Mbed-TLS:developmentfrom
bjwtaylor:unsafe-macros
Open

Add parentheses to unsafe macros#716
bjwtaylor wants to merge 3 commits into
Mbed-TLS:developmentfrom
bjwtaylor:unsafe-macros

Conversation

@bjwtaylor
Copy link
Copy Markdown
Contributor

@bjwtaylor bjwtaylor commented Mar 16, 2026 •
edited by gilles-peskine-arm
Loading

Description

Add parentheses to unsafe macros resolves #712

PR checklist

  • changelog provided | not required because: No public changes
  • framework PR not required
  • TF-PSA-Crypto development PR provided Add parentheses to unsafe macros #716
  • TF-PSA-Crypto 1.1 PR TODO
  • mbedtls development PR not required because: only crypto changes
  • mbedtls 4.1 PR not required because: only crypto changes
  • mbedtls 3.6 PR TODO
  • tests not required because: No changes

Add parentheses to unsafe macros
ebda5da 
Signed-off-by: Ben Taylor <ben.taylor@linaro.org>
@bjwtaylor bjwtaylor marked this pull request as draft March 16, 2026 15:24
irwir
irwir reviewed Mar 16, 2026
View reviewed changes
Comment thread include/psa/crypto_extra.h Outdated
#define PSA_PAKE_INPUT_SIZE(alg, primitive, input_step) \
(PSA_ALG_IS_JPAKE(alg) && \
primitive == PSA_PAKE_PRIMITIVE(PSA_PAKE_PRIMITIVE_TYPE_ECC, \
(PSA_ALG_IS_JPAKE((alg)) && \
Copy link
Copy Markdown
Contributor

@irwir irwir Mar 16, 2026

Choose a reason for hiding this comment

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

Multiple parentheses not required
(PSA_ALG_IS_JPAKE(alg) &&

Copy link
Copy Markdown
Contributor Author

@bjwtaylor bjwtaylor Mar 17, 2026

Choose a reason for hiding this comment

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

Should now be resolved.

irwir
irwir reviewed Mar 16, 2026
View reviewed changes
Comment thread drivers/builtin/src/ecp_curves.c Outdated
#else /* 64-bit */

#define MAX32 N->n * 2
#define MAX32 ((N->n) * 2)
Copy link
Copy Markdown
Contributor

@irwir irwir Mar 16, 2026

Choose a reason for hiding this comment

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

N->n is not a formal parameter of the macro
(N->n * 2)

Copy link
Copy Markdown
Contributor Author

@bjwtaylor bjwtaylor Mar 17, 2026

Choose a reason for hiding this comment

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

Should now be resolved.

irwir
irwir reviewed Mar 16, 2026
View reviewed changes
Comment thread drivers/builtin/src/ecp_curves_new.c Outdated
#else /* 64 bit */

#define MAX32 X_limbs * 2
#define MAX32 ((X_limbs) * 2)
Copy link
Copy Markdown
Contributor

@irwir irwir Mar 16, 2026

Choose a reason for hiding this comment

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

The same as above
(X_limbs * 2)

Copy link
Copy Markdown
Contributor Author

@bjwtaylor bjwtaylor Mar 17, 2026

Choose a reason for hiding this comment

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

Should now be resolved.

irwir
irwir reviewed Mar 16, 2026
View reviewed changes
Comment thread include/psa/crypto_extra.h Outdated
#define PSA_PAKE_OUTPUT_SIZE(alg, primitive, output_step) \
(PSA_ALG_IS_JPAKE(alg) && \
primitive == PSA_PAKE_PRIMITIVE(PSA_PAKE_PRIMITIVE_TYPE_ECC, \
(PSA_ALG_IS_JPAKE((alg)) && \
Copy link
Copy Markdown
Contributor

@irwir irwir Mar 16, 2026

Choose a reason for hiding this comment

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

The same, (PSA_ALG_IS_JPAKE(alg) &&

Copy link
Copy Markdown
Contributor Author

@bjwtaylor bjwtaylor Mar 17, 2026

Choose a reason for hiding this comment

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

Should now be resolved.

@irwir
Copy link
Copy Markdown
Contributor

irwir commented Mar 16, 2026

No bugs, only some extra parentheses. Technically, H macro also could use less parentheses, but now the order of calculations would be easier to read without recalling operator precedence in C.

Ben Taylor added 2 commits March 17, 2026 08:30
Remove surplus parentheses
733ed07 
Signed-off-by: Ben Taylor <ben.taylor@linaro.org>
Correct code style!
ee1e0b9 
Signed-off-by: Ben Taylor <ben.taylor@linaro.org>
@irwir
Copy link
Copy Markdown
Contributor

irwir commented Mar 17, 2026

The code is good now.
On the subject of style, backslashes as line continuations are not aligned, though it is everywhere in the sources.
Maybe alignment automation should be considered.

@bjwtaylor bjwtaylor added 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 size-xs Estimated task size: extra small (a few hours at most) priority-low Low priority - this may not receive review soon labels Mar 18, 2026
@bjwtaylor bjwtaylor marked this pull request as ready for review March 18, 2026 07:55
@davidhorstmann-arm davidhorstmann-arm moved this to Triage in in Community Mar 23, 2026
@davidhorstmann-arm davidhorstmann-arm moved this from Triage in to Scoped in Community Mar 23, 2026
@bjwtaylor bjwtaylor moved this from Scoped to Next 3 items in Community Mar 24, 2026
@davidhorstmann-arm davidhorstmann-arm moved this from Next 3 items to Scoped in Community Mar 25, 2026
@davidhorstmann-arm davidhorstmann-arm moved this from Scoped to Next 3 items in Community Mar 25, 2026
@gilles-peskine-arm gilles-peskine-arm self-requested a review April 10, 2026 15:26
@gilles-peskine-arm gilles-peskine-arm added priority-medium Medium priority - this can be reviewed as time permits needs-backports Backports are missing or are pending review and approval. and removed priority-low Low priority - this may not receive review soon labels Apr 10, 2026
gilles-peskine-arm
gilles-peskine-arm approved these changes Apr 10, 2026
View reviewed changes
Copy link
Copy Markdown
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.

LGTM. There are more unhygienic macros but we don't need to fix them all in one go.

@davidhorstmann-arm davidhorstmann-arm moved this from Next 3 items to Stalled in Community May 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gilles-peskine-arm gilles-peskine-arm gilles-peskine-arm approved these changes

+1 more reviewer

@irwir irwir irwir left review comments

Reviewers whose approvals may not affect merge requirements

At least 2 approving reviews are required to merge this pull request.

Assignees

No one assigned

Labels

needs-backports Backports are missing or are pending review and approval. 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 priority-medium Medium priority - this can be reviewed as time permits size-xs Estimated task size: extra small (a few hours at most)

Projects

Community
Status: Stalled

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Unsafe macros

4 participants

@bjwtaylor @irwir @gilles-peskine-arm @davidhorstmann-arm