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

Bugfix/update win crypto apis #8047

Merged
merged 33 commits into from
Sep 25, 2023
Merged

Bugfix/update win crypto apis #8047

merged 33 commits into from
Sep 25, 2023

Conversation

minosgalanakis
Copy link
Contributor

@minosgalanakis minosgalanakis commented Aug 9, 2023 •
edited
Loading

Description

This PR is a continuation of #730 and #1453 based on latest development. Resolves #1227

PR checklist

Please tick as appropriate and edit the reasons (e.g.: "backport: not needed because this is a new feature")

  • changelog provided.
  • backport Not required. As discussed, we are not introducing new dependencies on LTS unless really needed.
  • tests Not required. We are not testing in UWP but have tests in place for standard windows builds.

@minosgalanakis minosgalanakis added component-crypto Crypto primitives and low-level interfaces component-platform Portability layer and build scripts size-s Estimated task size: small (~2d) priority-very-high Highest priority - prioritise this over other review work labels Aug 9, 2023
@minosgalanakis minosgalanakis force-pushed the bugfix/update_win_crypto_apis branch 2 times, most recently from 65475fa to 387d00b Compare August 9, 2023 17:54
tom-cosgrove-arm
tom-cosgrove-arm reviewed Aug 9, 2023
View reviewed changes
ChangeLog Outdated Show resolved Hide resolved
@minosgalanakis minosgalanakis added needs-work needs-ci Needs to pass CI tests labels Aug 10, 2023
@minosgalanakis minosgalanakis force-pushed the bugfix/update_win_crypto_apis branch from 387d00b to e95c560 Compare August 11, 2023 14:53
@minosgalanakis minosgalanakis force-pushed the bugfix/update_win_crypto_apis branch from e95c560 to fd589be Compare August 11, 2023 15:53
@minosgalanakis minosgalanakis 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 and removed needs-ci Needs to pass CI tests labels Aug 14, 2023
daverodgman
daverodgman requested changes Aug 16, 2023
View reviewed changes
library/entropy_poll.c Outdated Show resolved Hide resolved
library/x509_crt.c Outdated Show resolved Hide resolved
@minosgalanakis minosgalanakis force-pushed the bugfix/update_win_crypto_apis branch 2 times, most recently from ffe8841 to 26b12ed Compare August 21, 2023 09:46
@davidhorstmann-arm davidhorstmann-arm self-requested a review August 23, 2023 09:30
@davidhorstmann-arm davidhorstmann-arm removed the needs-reviewer This PR needs someone to pick it up for review label Aug 23, 2023
@minosgalanakis minosgalanakis force-pushed the bugfix/update_win_crypto_apis branch from 2bb324a to 59108d3 Compare September 25, 2023 13:12
tom-cosgrove-arm
tom-cosgrove-arm reviewed Sep 25, 2023
View reviewed changes
library/x509_crt.c Outdated
* ANSI codepage. The input string is always described in BYTES and the
* output length is described in WCHARs.
*/
w_ret = MultiByteToWideChar(CP_ACP, 0, filename, length_as_int, szDir,
Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-arm Sep 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Re-reading this change, I'm not sure why we're bothering with the whole length_as_int thing.

This would be a concern if len wouldn't fit in an int, but we actually know that len <= MAX_PATH - 3, where MAX_PATH is 260 or 32,767 (https://learn.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation?tabs=registry)

So the cast in the (original) line 1559 is totally fine.

We also do a similar cast in (new) line 1588 - admittedly this is now "length remaining in buffer", which is a horrible re-use of variable with opposite meaning, but if we are happy with the cast at 1588 we should be happy with the cast at 1559.

(And fewer changes => better, IMO)

Supporting "Starting in Windows 10, version 1607, MAX_PATH limitations have been removed from common Win32 file and directory functions" would be a bigger, separate, set of changes

tom-cosgrove-arm
tom-cosgrove-arm reviewed Sep 25, 2023
View reviewed changes
library/x509_crt.c Outdated
@@ -61,6 +61,7 @@
#if defined(_WIN32) && !defined(EFIX64) && !defined(EFI32)
#define WIN32_LEAN_AND_MEAN
#include <windows.h>
#include <intsafe.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need this change :) or can't we bear the thought of another update? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's see if it clears CI, and I could try to push a last commit to remove that. Re reviewing an include could be simple.

Copy link
Contributor

Choose a reason for hiding this comment

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

(note that the only functional changes to this file now are the #if changes)

davidhorstmann-arm
davidhorstmann-arm previously approved these changes Sep 25, 2023
View reviewed changes
Copy link
Contributor

@davidhorstmann-arm davidhorstmann-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, thanks!

@minosgalanakis
x509_crt: Removed unused intsafe.h
2108775 
Signed-off-by: Minos Galanakis <[email protected]>
paul-elliott-arm
paul-elliott-arm previously approved these changes Sep 25, 2023
View reviewed changes
Copy link
Member

@paul-elliott-arm paul-elliott-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 - if you want to push a commit to remove the unnecessary include, then can quickly re-review

@paul-elliott-arm paul-elliott-arm added approved Design and code approved - may be waiting for CI or backports needs-ci Needs to pass CI tests and removed needs-review Every commit must be reviewed by at least two team members, labels Sep 25, 2023
@daverodgman daverodgman added needs-review Every commit must be reviewed by at least two team members, and removed approved Design and code approved - may be waiting for CI or backports labels Sep 25, 2023
paul-elliott-arm
paul-elliott-arm approved these changes Sep 25, 2023
View reviewed changes
Copy link
Member

@paul-elliott-arm paul-elliott-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

davidhorstmann-arm
davidhorstmann-arm approved these changes Sep 25, 2023
View reviewed changes
Copy link
Contributor

@davidhorstmann-arm davidhorstmann-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

@paul-elliott-arm paul-elliott-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Sep 25, 2023
tom-cosgrove-arm
tom-cosgrove-arm approved these changes Sep 25, 2023
View reviewed changes
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 - thanks

@paul-elliott-arm paul-elliott-arm added this pull request to the merge queue Sep 25, 2023
Merged via the queue into Mbed-TLS:development with commit fc9fc33 Sep 25, 2023
This was referenced Oct 27, 2023
Merged
Open
@wangyoucao577 wangyoucao577 mentioned this pull request Jan 20, 2024
Merged
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 left review comments

@daverodgman daverodgman daverodgman left review comments

@tom-cosgrove-arm tom-cosgrove-arm tom-cosgrove-arm approved these changes

@davidhorstmann-arm davidhorstmann-arm davidhorstmann-arm approved these changes

@paul-elliott-arm paul-elliott-arm paul-elliott-arm approved these changes

Assignees
No one assigned
Labels
approved Design and code approved - may be waiting for CI or backports component-crypto Crypto primitives and low-level interfaces component-platform Portability layer and build scripts needs-ci Needs to pass CI tests priority-very-high Highest priority - prioritise this over other review work size-s Estimated task size: small (~2d)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Linker error on Windows UWP
8 participants
@minosgalanakis @tom-cosgrove-arm @daverodgman @gilles-peskine-arm @paul-elliott-arm @davidhorstmann-arm @kevinmkane @simonbutcher