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

der: add BmpString #1164

Merged
merged 2 commits into from
Jul 24, 2023
Merged

der: add BmpString #1164

merged 2 commits into from
Jul 24, 2023

Conversation

tarcieri
Copy link
Member

Adds initial support for a Basic Multilingual Plane (a.k.a. UCS-2) string type.

Adds initial support for a Basic Multilingual Plane (a.k.a. UCS-2)
string type.
@baloo
Copy link
Member

baloo commented Jul 24, 2023

It looks good, the one change I'd like to introduce would be to avoid silently accepting chars that encode over more than 2 bytes (which this currently does).

I'd prefer that to be a legitimate choice by the consumer of the API if possible (hence the hazmat flag and the new_unchecked naming), something like: 8b6f5f5

@tarcieri
Copy link
Member Author

@baloo I'm not terribly well versed in UCS-2/UTF-16. Is the issue there unpaired surrogates or something different?

Would composing from_utf8 in terms of from_ucs2 catch it, or do we need to check the value of each decoded character (in from_ucs2) to make sure it doesn't exceed u16::MAX?

@carl-wallace
Copy link
Contributor

Looks good to me (no opinion re: encoding to more than two bytes). I dropped it into the work-in-progress P12 code and it worked fine for the (simple) friendly name attributes in that set.

@baloo
Copy link
Member

baloo commented Jul 24, 2023

@tarcieri I thought there were issues with the back and forth between str and BmpString but there isn't any. Disregard my previous comment.

@tarcieri
Copy link
Member Author

@baloo I am a little curious if the output will be something other than valid BMP, e.g. codepoints greater than 65535. I suppose I could still check for that?

@tarcieri
Copy link
Member Author

tarcieri commented Jul 24, 2023

Went ahead and added additional checks in 8d0ed28 e87a10b

@tarcieri tarcieri merged commit f8f2cbd into master Jul 24, 2023
104 checks passed
@tarcieri tarcieri deleted the der/bmpstring branch July 24, 2023 18:00
baloo added a commit to baloo/formats that referenced this pull request Aug 7, 2023
Added
- `bytes` feature ([RustCrypto#1156])
- impl `RefToOwned`/`OwnedToRef` for `&[u8]`/`Box<[u8]>` ([RustCrypto#1188])
- `BmpString` ([RustCrypto#1164])

Changed
- no-panic cleanup ([RustCrypto#1169])
- Bump `der_derive` dependency to v0.7.2 ([RustCrypto#1192])

[RustCrypto#1156]: RustCrypto#1156
[RustCrypto#1164]: RustCrypto#1164
[RustCrypto#1169]: RustCrypto#1169
[RustCrypto#1188]: RustCrypto#1188
[RustCrypto#1192]: RustCrypto#1192

Signed-off-by: Arthur Gautier <baloo@superbaloo.net>
@baloo baloo mentioned this pull request Aug 7, 2023
baloo added a commit that referenced this pull request Aug 7, 2023
Added
- `bytes` feature ([#1156])
- impl `RefToOwned`/`OwnedToRef` for `&[u8]`/`Box<[u8]>` ([#1188])
- `BmpString` ([#1164])

Changed
- no-panic cleanup ([#1169])
- Bump `der_derive` dependency to v0.7.2 ([#1192])

[#1156]: #1156
[#1164]: #1164
[#1169]: #1169
[#1188]: #1188
[#1192]: #1192

Signed-off-by: Arthur Gautier <baloo@superbaloo.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants