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

OCB-AES: improve the calculation of double mask #6667

Closed
wants to merge 1 commit into from
Closed

OCB-AES: improve the calculation of double mask #6667

wants to merge 1 commit into from

Conversation

DesWurstes
Copy link
Contributor

@DesWurstes DesWurstes commented Jul 7, 2018

Note that this improves performance.

CLA: trivial

Checklist
  • documentation is added or updated
  • tests are added or updated

@@ -74,7 +74,8 @@ static void ocb_double(OCB_BLOCK *in, OCB_BLOCK *out)
*/
mask = in->c[0] & 0x80;
mask >>= 7;
mask *= 135;
mask *= -1;
mask &= 135;
Copy link
Contributor

Choose a reason for hiding this comment

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

If one aims for multiplication removal, then I'd rather suggest mask = (0 - mask) & 135. It would be more readable, for following reason. There are implicit type conversions that take place here, and multiplication by -1 triggers more elaborate mental path to confirm that course of operations is free from undefined behaviour as defined by language standard. I would also suggest to replace 135 with hexadecimal for better readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -74,7 +74,7 @@ static void ocb_double(OCB_BLOCK *in, OCB_BLOCK *out)
*/
mask = in->c[0] & 0x80;
mask >>= 7;
mask *= 135;
mask = -mask & 0x87;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested (0 - mask) was not a coincidence. Microsoft compiler complains about unary minus with unsigned type. It's a warning, but we attempt to minimize warnings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done again.

@dot-asm dot-asm added the approval: review pending This pull request needs review by a committer label Jul 8, 2018
@dot-asm
Copy link
Contributor

dot-asm commented Jul 8, 2018

Editorial note. The modified code is cipher-agnostic, yet subject speaks of AES. If I end up committing this I intend to replace "AES-OCB" in subject with "modes/ocb128.c".

@richsalz richsalz added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Jul 8, 2018
levitte pushed a commit that referenced this pull request Jul 9, 2018
CLA: trivial

Reviewed-by: Andy Polyakov <appro@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from #6667)
@dot-asm
Copy link
Contributor

dot-asm commented Jul 9, 2018

Merged. Thanks.

@dot-asm dot-asm closed this Jul 9, 2018
@DesWurstes DesWurstes deleted the patch-1 branch July 9, 2018 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: done This pull request has the required number of approvals
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants